Skip unchanged trimmable typemap assembly emission - #12599
Conversation
Reduce scanner and emitter allocations by caching metadata, avoiding unused MCW method scans, and eliminating intermediate buffers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Persist versioned model fingerprints so incremental builds only emit typemap PE assemblies whose final model changed, while retaining alias-owner and root-reference invalidation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs — ReadTypeMapFingerprints() treats the fingerprints file as… |
What changed in this PR
Adds an incremental-build path to trimmable typemap generation by fingerprinting the typemap content model and skipping PE emission when fingerprints and on-disk outputs are unchanged. This fits into the build pipeline by reducing repeated work in GenerateTrimmableTypeMap while preserving byte-identical outputs when generation does occur.
Changes:
- Introduces per-assembly and root typemap incremental fingerprints (including generator identity + emitter-consumed model fields) and a callback to decide whether to emit each output.
- Persists typemap fingerprints to disk and wires the file into the MSBuild task + targets so subsequent builds can skip unchanged PE emission.
- Adds/extends unit + task tests to validate skipping behavior, byte identity, and invalidation scenarios (alias ownership + assembly-set changes).
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TrimmableTypeMapGeneratorTests.cs | Adds generator-level tests covering skip/all-skip, byte identity, alias invalidation, and root regeneration on assembly-set changes. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GenerateTrimmableTypeMapTests.cs | Extends task test to verify second run skips PE emission (and preserves output timestamp) when fingerprints are persisted. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs | Persists and consults typemap fingerprints to skip PE emission; updates assembly writing flow to support “skipped but still output” assemblies. |
| src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets | Wires the fingerprints file into the task and ensures it’s tracked for cleaning via @(FileWrites). |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs | Adds an optional callback for incremental emission decisions; computes per-assembly/root fingerprints before PE emission. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyGenerator.cs | Refactors generation to separate model creation/fingerprinting from PE emission. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/MetadataHelper.cs | Implements incremental fingerprint computation for per-assembly and root typemap outputs. |
Persist versioned model fingerprints so incremental builds only emit typemap PE assemblies whose final model changed, while retaining alias-owner and root-reference invalidation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
fa94514 to
51e0fdf
Compare
…sival-incremental-typemap-emission
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Reject
Found 1 error in the incremental-clean integration. The model-first fingerprinting and selective PE emission are otherwise well-structured, with good focused coverage for fingerprint stability, alias ownership, root invalidation, and byte-identical output.
CI is still in progress: 42 checks have passed, with the Java.Interop macOS lane running and the aggregate check queued. No failures are currently reported.
Generated by Android PR Reviewer for #12599 · gpt56 · 155.6 AIC · ⌖ 19.9 AIC · ⊞ 25.7K
Comment /review to run again
Keep the fingerprint cache registered during no-op builds and treat unreadable or malformed cache contents as a regeneration signal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Review summary
Verdict: Needs changes
I found no high-confidence correctness defects in the implementation. The fingerprints cover the emitter-consumed model, root membership invalidation, generator identity, runtime version, and shared-universe mode; skipped assemblies remain represented in the expected output set. I left one inline testing suggestion for the missing-output recovery path.
CI: Azure DevOps build 1575463 is red in Windows MSBuild lanes 1, 6, and 8 and macOS lanes 2, 3, and 10. The Azure CLI could not initialize its required writable profile in this environment, so I could not attribute those failures from the timeline/logs. These failures should be resolved or confirmed unrelated before merge.
Findings: 0 bugs · 0 warnings · 1 suggestion
Generated by Android PR Reviewer for #12599 · gpt56 · 290.9 AIC · ⌖ 9 AIC · ⊞ 25.7K
Comment /review to run again
| var secondWriteTime = File.GetLastWriteTimeUtc (typeMapPath); | ||
| Assert.AreEqual (firstWriteTime, secondWriteTime, | ||
| "Typemap assembly should NOT be rewritten when content hasn't changed."); | ||
| Assert.IsTrue (messages.Any (message => message.Message?.Contains ("_Mono.Android.TypeMap: unchanged, skipping emission", StringComparison.Ordinal) == true), |
There was a problem hiding this comment.
🤖 💡 Testing: Please also cover the persisted-cache recovery path where the fingerprint matches but the output DLL is missing. After the first execution, delete typeMapPath, execute the task again, and assert that the DLL is recreated rather than reported as unchanged. ShouldGenerateTypeMapAssembly explicitly treats a missing output as dirty, and that behavior is important because these fingerprints survive across builds.
Stacked on #12598.
Summary
Incremental benchmark
Release scratch harness over
TestFixtures.dll, median of 10 batches of 100 runs. Both paths perform assembly scanning and model construction; the incremental path skips unchanged per-assembly and root PE emission.Correctness
Microsoft.Android.Sdk.TrimmableTypeMap.Tests: 779 passed._Mono.Android.TypeMapPE emission and preserves the output timestamp.Full-build task/integration validation was unavailable because this worktree has no local Android SDK build (
bin/Debug/dotnet/dotnet/bin/Release/dotnet/dotnet). A direct build attempt reached missing bootstrap/local environment prerequisites (including no usable JDK), so no full SDK build was started.