Repository navigation
[CoreCLR] Move assembly-store runtime state out of generated native data - #12972
Conversation
|
@dalexsoto review |
|
@simonrozsival I haven't submitted a review for this request: The PR is currently marked as a draft. I am deferring code review until it is marked ready for review; no draft head is being recorded as reviewed. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Native startup, concurrent decompression, and Full-MSBuild task-host behavior still need targeted runtime validation.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Moves CoreCLR assembly-store mutable state from generated native data into runtime-owned host allocations, advancing #12940 while preserving XABA/XAZS formats.
Changes:
- Allocates and initializes runtime assembly/decompression state from store metadata.
- Migrates compression metadata tasks into
Microsoft.Android.Build.Tasks. - Removes obsolete compressed-assembly LLVM generation and adds regression coverage.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets |
Rewires compression tasks and removes LLVM inputs. |
src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj |
Links shared assembly helpers. |
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs |
Removes migrated helpers. |
src/Xamarin.Android.Build.Tasks/Utilities/CompressedAssembliesNativeAssemblyGenerator.cs |
Deletes obsolete LLVM generator. |
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGenerator.cs |
Removes generated store runtime globals. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/TrimmableTypeMapBuildTests.cs |
Validates runtime slot capacity from store headers. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateNativeApplicationConfigSourcesTests.cs |
Checks removed globals. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs |
Checks compressed LLVM artifacts are absent. |
src/Xamarin.Android.Build.Tasks/Tasks/PrepareAbiItems.cs |
Removes compressed-source mode. |
src/Xamarin.Android.Build.Tasks/Tasks/GenerateCompressedAssembliesNativeSourceFiles.cs |
Deletes obsolete generation task. |
src/native/clr/xamarin-app-stub/application_dso_stub.cc |
Removes stub store-state symbols. |
src/native/clr/include/xamarin-app.hh |
Removes exported store-state declarations. |
src/native/clr/include/host/assembly-store.hh |
Adds runtime-owned state fields. |
src/native/clr/host/assembly-store.cc |
Allocates, initializes, and consumes runtime state. |
src/Microsoft.Android.Build.Tasks/Utilities/MonoAndroidHelper.Assemblies.cs |
Hosts shared assembly grouping helpers. |
src/Microsoft.Android.Build.Tasks/Utilities/CompressedAssemblyInfo.cs |
Simplifies compression metadata. |
src/Microsoft.Android.Build.Tasks/Utilities/AssemblyCompressor.cs |
Updates documentation. |
src/Microsoft.Android.Build.Tasks/Utilities/AssemblyCompression.cs |
Moves compression helpers into modern tasks. |
src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/CompressionMetadataTests.cs |
Tests sparse indices and changed payloads. |
src/Microsoft.Android.Build.Tasks/Tasks/CollectCompressedAssemblyInfo.cs |
Adds pre-trimming metadata producer. |
src/Microsoft.Android.Build.Tasks/Tasks/CollectAssemblyFilesToCompress.cs |
Consumes stable descriptor metadata. |
Derive runtime slots and sparse compression descriptor capacity from unchanged XABA/XAZS metadata. Retain process-lifetime image buffers, read-only payloads and the writable-copy workaround. Move the paired compression metadata tasks and typed registry into the modern task assembly, removing only obsolete generated store globals and the compression LLVM stream. Part of #12940. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Drop the new boilerplate registration, disabled-option, missing-file and duplicate helper coverage. Keep sparse pre-trim descriptor identity with cached frame reuse and changed post-trim payload length/roundtrip regressions, with stronger assertions and smaller helpers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…d state Resolve the immutable implementation closure in isolated resolve-only RID instances and keep the paired metadata registry in the outer modern task host. Invalidate mismatched cached XAZS identities and retain unchanged compressed bytes/timestamps using the existing pooled stream/file helpers. Preserve process-lifetime returned images across sequential reconfiguration and reject invalid allocation metadata explicitly. Keep only the focused public-file and concurrent-load regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
dalexsoto
left a comment
There was a problem hiding this comment.
The complete 21-file source and producer-to-runtime review found no high-confidence source blocker in sparse descriptor capacity, publication/decompression/lifetime, paired compression metadata, shipped task-host behavior, or removal/packaging/Clean parity. The heap/startup allocation and process-lifetime buffer tradeoffs and unchanged trusted XABA/XAZS pointer contract remain accepted.
One concrete platform blocker remains: this exact head cannot merge into current main. Fresh GitHub metadata reports mergeable=false, mergeable_state=dirty, and rebaseable=false; the associated current check is "Skipped due to merge conflicts." Please resolve those conflicts, then validate and review the resulting head. The successful exact-source build 1624677 does not clear the current conflict state. No individual conflict filename was supplied by the platform response, so none is invented here.
Missing Full-MSBuild/device/x86 test coverage was not converted into an unsupported code defect: the corresponding static contracts were inspected. Android builds/tests, Full-MSBuild integration, device allocator/concurrent-load execution and the prepared smoke artifacts were not run by this reviewer.
cc4549f to
5f27a17
Compare
Strengthen the existing concurrent first-load regression instead of adding another fixture or ABI matrix. Read the actual installed package with the shared store reader, require both deferred fixtures to be compressed with sparse pre-trim indices, and invoke their preserved getter on each loader thread so executable image bytes are exercised, not just metadata. Reflection suppressions are limited to the explicitly rooted fixture assemblies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@dalexsoto The merge-conflict blocker in your review is addressed: the branch was rebased onto main |
dalexsoto
left a comment
There was a problem hiding this comment.
The merge conflicts are resolved. The complete 23-file source review and separate integration/completeness/final sweeps preserve the accepted heap/lifetime, trusted XABA/XAZS/ELF contract, NET task-host routing and behavioral-test scope. Newly available current-head CI evidence exposed one packaging blocker before this consolidated decision.
Include generated composite R2R images in the compression identity universe (resolve-only change:158-166). Registration now receives only the pre-publish resolve-only inputs. The exact pinned SDK subsequently creates and publishes a new Foo.r2r.dll composite, so its final DestinationSubPath key is absent. The consumer skips compression when no descriptor exists, and store creation falls back to the raw image. Exact base registered final resolved assemblies after publish, including generated R2R identities. Preserve stable pre-trim identities while also assigning stable, unique identities to final-publish-generated images; retain sparse capacity and cached-header validation. The strengthened device regression disables R2R, and the existing R2R packaging test disables compression, so neither establishes this combination.
Current-source build 1624963 has three required APK-size failures. The two Simple cases report stores growing from 2,437,600/2,437,584 to 5,791,160 bytes and APKs from 6,522,299 to 6,866,363 (+344,064; 5.28%). The Forms log reports APK growth from 19,331,980 to 21,760,908 (+2,428,928; 12.56%). These exceed the unchanged 3% package/5% content checks. The source compression omission is independently established; every observed byte is not exclusively attributed without matched APK/base inventories. Correct the compression path and validate the resulting sizes before any substantiated policy-based reference update, rather than unconditionally accepting the increases. This is not a challenge to the accepted runtime-heap cost or an ELF-format change.
The separately investigated project-instance registry collision is unchanged in exact base as well as cc4549...; no migration-specific new exposure was established, so it is not retained as a blocker for this PR. No product build, Windows host, device or CI rerun was performed by this review.
Crossgen2 emits a composite image after the pre-trim inventory is resolved. Append generated package identities after that inventory without renumbering pre-trim descriptors, so the composite does not silently bypass XAZS compression. Extend the existing sparse/reuse roundtrip regression and update only the smaller libxamarin-app references caused by removing generated writable state. Keep store/APK references and regression thresholds unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use token-aware async HTTP stream reads and propagate cancellation before installing archives or reporting success. Previously downloads could run beyond the deadline and installation would be skipped with a successful task result. Preserve the product ten-minute default; allow thirty minutes only for the existing fresh SDK/JDK/NDK integration case and assert that a forced timeout fails explicitly before its normal install. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@dalexsoto Addressed the composite-R2R blocker in your latest review with commit The producer now receives both the original resolve-only pre-trim closure and final packaged assemblies. Generated identities such as All four exact An actual composite-R2R compressed APK was built for arm/arm64/x64, inspected to verify an XAZS composite frame at appended index193 with19entries/capacity194, and cold-started on an owned ARM64/API35 emulator, executing The PR body contains exact commands/results and platform gaps. New build1625333 targets the new head and is still running; I am not treating the old CI result as new-head green. Please re-review the updated head. |
dalexsoto
left a comment
There was a problem hiding this comment.
The composite-R2R omission is fixed: final generated package identities append after the original pre-trim closure without renumbering existing descriptors. The resolve/publish/compression/cache/store/runtime path and sparse capacity are consistent. Installer cancellation now reaches stream reads and prevents installation/success reporting after timeout, with the product's ten-minute default preserved.
The complete current 29-path source review, including all three rename origins, separate integration/completeness pass and final sweep found no remaining blocker. Package/store references and the 3%/5% gates remain unchanged; only the intentionally smaller app-native reference is updated. Exact-source build 1625333 succeeds. Accepted heap/buffer lifetime, trusted XABA/XAZS/ELF and NET task-host contracts remain intact; the exact-base-preexisting registry issue stays withdrawn. Product, device and Windows execution receipts remain attributed rather than claimed as reviewer execution.


Move CoreCLR assembly-store mutable state out of generated app-native data and into the runtime host. Runtime slots and decompression storage are derived from immutable XABA v3 descriptors and existing XAZS headers, not duplicated in writable LLVM-generated data.
The store writer, ELF layout and payload format are unchanged:
libassembly-store.soremains data-only with onePF_R PT_LOAD._assembly_storethroughdlopen/dlsymandconfigure_from_payload(pointer, path)remain the loader contract. No writable/BSS segment, size/end symbol or ELF walk is introduced.The host preserves lazy synchronized decompression, name/hash-collision disambiguation, uncompressed writable copies and process-lifetime returned buffers, including previous buffers across sequential reconfiguration. Sparse capacity is checked maximum XAZS index + 1, not trimmed entry count. Sizes are overflow checked and allocation/metadata failures explicit; configuration never eagerly decompresses images.
Only
compressed_assemblies.<abi>.ll/.oand its generation/source/object/link plumbing are removed, together with generatedcompressed_assembly_count,compressed_assembly_descriptors,uncompressed_assemblies_data_size,uncompressed_assemblies_data_buffer,assembly_store_bundled_assembliesandassembly_store.libxamarin-app.so, its stub, environment/DSO/runtime-property/JNI-remapping LLVM streams, application config and official NDK routing remain.The paired metadata producer/consumer and typed assembly-local registry live in
Microsoft.Android.Build.Tasks. Resolve-only RID instances provide the pre-trim implementation closure to the same outer modern task host via supportedMSBuildRuntime="NET"routing. Crossgen2-generated package images are appended after that closure without renumbering its identities. Cached XAZS identities are checked, unchanged compressed bytes preserve timestamps via existing pooled stream/file helpers, and outputs remain registered for Clean.Part of #12940, not closing that umbrella issue. Current head
975357e6f3ea20d54e7e4326be762e40c09e47b0is based on main99dda3d8da8ecfb0802641fdaa9389f841112c53. Main's managed writer, NativeAOT bootstrap, typemap/runtime removal, NDK routing, retained-rule pipeline and merged R8 remapping are preserved. No unmerged LLVM/bootstrap PoC or new branch stack was imported.Review updates
All three GitHub inline threads have replies and are resolved. The earlier merge-conflict blocker was fixed by the user-requested rebase. The same existing concurrent-load device regression now verifies actual packaged sparse XAZS indices and both compressed deferred fixtures, then invokes their explicitly rooted
LoadProbe.Valueon all16loading threads. No additional fixture, ABI matrix, generated-LLVM assertion suite or bespoke committed native harness was added. New coverage remains two portable compression regressions and one device case; existing installer integration coverage is extended rather than duplicated.CI failure fixes
Build 1624963 at
cd8ff4d9a3exposed two independently diagnosed roots:UnnamedProject.r2r.dllis synthesized by crossgen2, absent from the original pre-trim closure, and was silently packaged as a raw PE. The reproduced4.92MB image explained the simple store growth to5.79MB.75ff16fa70appends generated packaged identities after the original closure. All four exact failing simple/Forms, R8on/off size tests now pass. Only the expected smallerlibxamarin-app.soreferences are updated to9600bytes after removing writable generated state; store/APK reference sizes and thresholds are unchanged.install-deps.logshowed19minutes of SDK/JDK/NDK downloads, no installation, no cancellation error and Exit0 after the10minute token expired.975357e6f3makes stream reads cancellation-aware and propagates cancellation before installation/success. The product10minute default remains; only the existing fresh-install integration test explicitly allows30minutes and verifies a forced timeout fails before normal installation. This ceiling does not add a delay to successful installs.Fresh local validation
SDK/NDK/JDK/cache data were privately APFS-cloned; no donor task/native/test/obj/CMake outputs were treated as validation. Own tasks/targets, hosts/stubs, manifests and test assemblies were rebuilt; node reuse disabled.
make prepare CONFIGURATION=Releasefollowed bymake leeroy CONFIGURATION=Release MSBUILD_ARGS="-m:2 -nr:false"make allsolution compilation succeeded, but workload configuration needed those extra binding levels../dotnet-local.sh build src/native/native-clr.csproj -c Release -nr:false -m:2./dotnet-local.sh build src/Microsoft.Android.Build.Tasks/Microsoft.Android.Build.Tasks.csproj -c Release -nr:false./dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj -c Release -nr:false./dotnet-local.sh build src/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj -c Release -nr:false./dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj -c Release -nr:false./dotnet-local.sh test bin/TestRelease/net10.0/Xamarin.Android.Build.Tests.dll --filter 'Name~BuildReleaseArm64'apkdiff0.0.17../dotnet-local.sh test src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj -c Release --filter 'FullyQualifiedName~CompressionMetadataTests|FullyQualifiedName~AssemblyStoreElfWriterTests|FullyQualifiedName~WrapAssemblyStoresAsSharedLibrariesTests|FullyQualifiedName~DlopenAssemblyStoreGeneratorTests' -nr:false./dotnet-local.sh test bin/TestRelease/net10.0/Xamarin.Android.Build.Tests.dll --filter '(Name~IncrementalCompression&Name~CoreCLR)|(Name~MissingSatelliteAssembly&Name~CoreCLR)|(Name~InstallAndroidDependenciesTest&Name~GoogleV2&Name~CoreCLR)'./dotnet-local.sh test bin/TestRelease/net10.0/Xamarin.Android.Build.Tests.dll --filter 'Name~InstallAndroidDependenciesTest&Name~Xamarin&Name~NativeAOT'Filter
|characters are literal separators, not shell pipelines. Privateapkdiffwas installed only after the selected test failed because its executable was missing.Corrected stores measured 2,436,776bytes for simple apps, versus the existing2,437,584/2,437,600 references; Forms stores measured 10,772,632/10,772,648bytes, matching existing10,771,592/10,772,704 references within unchanged gates. The simple APK returned to its exact6,522,299byte reference; Forms package-size gates passed. No size regression is hidden by increasing store baselines or tolerances.
The rebuilt production installer was also executed against a private real HTTP archive stream: a blocked response canceled at 60.5s with the existing localized timeout diagnostic and a failed task, not successful installation. A separate local transfer of the original cached platform-tools37.0.1archive completed and actually installed
adband its expectedsource.properties; the minimal platform-tools-only fixture produced the expected missing-command-line-tools license warning. Both local HTTP servers were cleaned up.Real packaging and runtime
All three shipping CoreCLR ABIs built an actual compressed composite ReadyToRun package using:
APP=/Users/simon/.copilot/session-state/f52efb88-5451-419d-9708-721a3d6d746a/files/package-app/AssemblyStorePackaging.csproj. Passed: each composite is an XAZS frame at appended index193, with19store entries and capacity194. A no-op build retained all57compressed files' exact bytes/nanosecond timestamps. Installed on uniquely owned ARM64/API35/4KiB emulator5580, the actual compressed composite executed a cold start through the resolved main Activity and loggedSTORE_PACKAGING_READY=17. That AVD and isolated adb5063 were stopped/deleted; shared devices were untouched.Earlier evidence remains distinct from these CI-fix checks: all four compressed/uncompressed APK/AAB variants built for arm/arm64/x64; actual trim-subset edits produced18entries/capacity193 and preserved48/57compressed outputs, followed by a no-op preserving all57. Existing R2R/Clean/config tests15passed. The strengthened sparse/concurrent executable-image device case passed on owned ARM64/API35/4KiB. Earlier ARM64/API35/4KiB and API36/16KiB execution covered compressed/uncompressed startup,16first-load threads, real allocator failure paths, sparse[0,17]/capacity18, hash collisions, writable copies, empty stores and sequential-reconfiguration image lifetime. The most recent16KiB AVD attempts failed before runtime and are not claimed as fresh16KiB coverage. Scratch native checks were not committed as test infrastructure.
Remaining coverage and CI
Windows exact installer execution, WindowsFullMSBuild and ARM32/x64 device execution were not available locally; Windows remains covered by CI, and arm/x64 were built/packaged. No startup-performance or memory improvement/parity is claimed.
Current-head Azure build1625333 is in progress, with
triggerInfo.pr.sourceSha=975357e6f3ea20d54e7e4326be762e40c09e47b0. The failed old build1624963 is not represented as current-head passing; no CI retry was used to debug the fixes.Pull Request
title and
description
should follow the
commit-messages.mdworkflow documentation, and in particular should include: