Reduce typemap signature encoder allocations - #12601
Closed
simonrozsival wants to merge 2 commits into
Closed
Conversation
Cache fixed activation and CreateInstance signatures so repeated PE emission avoids nested signature encoder delegates while preserving emitted bytes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Build the cached signatures through the existing SRM encoder so the optimization stays small and preserves the established encoding path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
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 |
|---|---|
 |
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs — |
What changed in this PR
This PR reduces allocations in the trimmable typemap PE emitter by caching two fixed method signatures that are emitted repeatedly (CreateInstance and the activation .ctor), and by extending PEAssemblyBuilder APIs to accept pre-encoded BlobHandle signatures so signature encoding can be done once and reused.
Changes:
- Cache and reuse the
CreateInstanceand activation-constructor signatures insideTypeMapAssemblyEmitter. - Add
PEAssemblyBuilderoverloads to emit method bodies and member refs using a pre-encodedBlobHandlesignature. - Add a unit test validating that a pre-encoded signature round-trips correctly through emission and metadata decoding.
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Adds coverage for emitting methods using a pre-encoded signature blob. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Caches two frequently-reused signature blobs to reduce repeated encoder allocations. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs | Introduces overloads that accept pre-encoded BlobHandle signatures for emission and member refs. |
Comment on lines
+420
to
+423
| encoder => { | ||
| encoder.OpCode (ILOpCode.Ldnull); | ||
| encoder.Return (returnsValue: true); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cache the two fixed signatures repeated for every emitted peer (
CreateInstanceand the activation constructor), and letPEAssemblyBuilderaccept a pre-encodedBlobHandle. This avoids rebuilding nested SRM signature callbacks while retaining the existing encoder and IL/max-stack paths.This is the third PR in #12598 -> #12599 -> this PR and must remain based on
simonrozsival-incremental-typemap-emission.Measurements
Release
net11.0harness, three warmups per process. The primary comparison used 21 alternating parent/final pairs for Microsoft.Android.Ref.36Mono.Android.dll(18 MiB, 8,820 peers) and 31 pairs forTestFixtures.dll.The small wall deltas are within observed process noise: the unchanged path does not execute the changed emission code but shows the same-sized delta. A separate 102-iteration in-process real-input comparison measured changed emission at 102.618 ms parent vs 102.632 ms final (effectively neutral), with allocations at 89.927 MiB vs 89.458 MiB.
Allocation traces over three changed real-input iterations showed sampled
Action<ParametersEncoder>allocations falling from 9.46 MiB to 2.54 MiB andAction<BlobEncoder>from 7.91 MiB to 4.45 MiB. CPU traces showed no attributable wall-time regression.Rejected experiments
BufferedStreamdid not recover the regression. Reverted, including staging/atomic-replacement code.CollectBaseConstructorChain, but the generic-context-safe cache produced no consistent real-input scan improvement and added about 0.012 MiB scan allocations. Reverted.PEStreamOptions.PrefetchMetadata: real changed median regressed from 249.055 ms to 264.390 ms; unchanged regressed from 128.780 ms to 227.844 ms, with effectively unchanged allocations. Reverted.Output identity
Parent and final aggregate SHA-256 hashes match:
D991AC653F162B8E7B461BE66CCB1643A6B0354452718CFE91222E6EA3B47D42E4D683D59AEF99603D8BAD51248A693168A7E71894556F3DD4493BAFA62726A71EA6E805D6375F0C5C726A3281CE085D26679CBE3F467ACE9517F7EE1B43F6A1D4A4A1CF9D4C4848CD97D9CCE1E2996DD8CAF1EF82E084E28FA9C4A83F621E41Validation
dotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj -c Release -v minimal: 780 passedTrimmableTypeMap.IntegrationTestswere not run because neitherbin/Debug/dotnet/dotnetnorbin/Release/dotnet/dotnetexists in this workspace.