Repository navigation
[CoreCLR] Reuse writable assembly-store images across probes - #13314
simonrozsival wants to merge 2 commits into
Conversation
Rejected assembly identities can repeatedly probe the same bundled image. Copying its uncompressed bytes on every probe leaks another full image, even though CoreCLR rejects the binding. Cache a writable image per physical runtime entry, using acquire/release publication and the existing startup-aware lock. Keep returned images alive across sequential store reconfiguration, as CoreCLR requires. Preserve the existing cached decompression behavior. Add sanitized production-loader tests for repeated and concurrent access, name aliases, writable data, and reconfiguration lifetime. Exercise rejected bindings on-device with raw and compressed assembly stores. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the writable assembly-image caching change.
No open issues 🎉
What changed in this PR
This PR reduces CoreCLR assembly-store allocations by reusing one writable image per physical entry, including when repeated probes request incompatible assembly versions.
Changes:
- Cache writable images with acquire/release publication and the existing startup-aware lock, sharing buffers across name aliases.
- Preserve returned images across sequential store reconfiguration while retaining compressed-image caching.
- Add native regression coverage and on-device rejected-version tests, with test setup documented in the catalog.
| File | Description |
|---|---|
tests/Mono.Android-Tests/Mono.Android-Tests/System/AssemblyStoreTests.cs |
Tests repeated rejected versions followed by successful assembly loads. |
tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj |
Includes the new device tests. |
src/native/clr/include/xamarin-app.hh |
Adds a writable-image pointer per runtime entry. |
src/native/clr/include/host/assembly-store.hh |
Updates mutable entry access and the shared mutex name. |
src/native/clr/host/assembly-store.cc |
Caches writable images and preserves their lifetime across reconfiguration. |
src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Resources/AssemblyStore/image-cache-probe.cc |
Exercises concurrency, aliases, writable data, reversed mappings, and retained buffers. |
src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj |
Copies native probe resources to test output. |
src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/AssemblyStoreImageCacheTests.cs |
Builds and runs the production-loader harness with sanitizers. |
.github/skills/tests/references/test-catalog.md |
Documents host-test prerequisites and device-test options. |
Review effort: Balanced
Remove the new native and managed regression fixtures and their project and catalog entries. Keep the production-only image reuse improvement, without adding test infrastructure for the artificial repeated-bind case. Restore the original includes and Debug guard, which no longer need to support compiling the loader in the removed host test harness. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
0 errors · 1 warning · 0 suggestions. The inline warning requests automated regression coverage for repeated and concurrent uncompressed-image probes; ordinary successful-load tests do not distinguish this fix from the previous implementation.
The implementation is otherwise focused: the new cache field is zero-initialized by the existing calloc, the initialized image is published with acquire/release semantics and the existing double-checked locking pattern, and retaining images when metadata is replaced preserves the documented CoreCLR lifetime contract. I found no confirmed correctness defect in the changed code.
CI: All 41 GitHub checks report success, including the aggregate dotnet-android check for Azure build 1654380. Independent Azure build/timeline verification was blocked: the build query required authentication, and the parallel timeline query failed during CLI initialization. No local builds or device tests were run for this review.
Generated by Android PR Reviewer for #13314 · copilot · gpt61 · 21.7 AIC · ⌖ 4.85 AIC · ⊞ 20K
Comment /review to run again
| if (rw_pointer == nullptr) [[unlikely]] { | ||
| Helpers::abort_application (LOG_ASSEMBLY, "Unable to allocate writable assembly data"); | ||
| // the assembly data to a read-write area. Cache it even when the binder rejects the | ||
| // requested identity, so subsequent probes reuse the same process-lifetime image. |
There was a problem hiding this comment.
🤖
{Rule: Bug fixes need regression tests; test assertions must be specific}
AssemblyStore::get_assembly_data()copies uncompressed assembly images into writable memory for the existing MAUI workaround. Unlike the compressed path, it does not remember the returned buffer: another probe for the same entry allocates and copies the image again, retaining both copies for the process lifetime.This can be exercised by requesting the same bundled assembly with different incompatible versions, causing CoreCLR to probe the image and reject each binding. That is an artificial stress case, not an established real-world workload. Successful loads and repeated requests for the same failed identity generally avoid repeated probing; this change is not intended to explain memory growth in ordinary applications.
Keep the writable pointer in the existing
runtime_assembliesentry and return it on subsequent probes, using the size already stored in the descriptor. Publish the initialized pointer with acquire/release semantics under the existing startup-aware lock so concurrent first probes share one copy. Preserve writable image semantics and retain returned buffers across sequential store reconfiguration, as CoreCLR requires. The compressed path continues to reuse its existing decompression buffer.make prepareandmake leeroypassed during initial validation. Earlier Debug/Release device probes against the rebuilt SDK confirmed image reuse. After reducing the change to the three native implementation files,make allpassed with one build-tool Git metadata warning and no errors. No new tests are included, and the stress probes were not rerun after removing the test additions.