[LlvmIrGenerator] ~4x faster type map generation - #12279
Conversation
WriteStringBlobArray() emitted each byte of a string blob by calling WriteType() and WriteValue(). Both take a Type, so every byte performed reflection (Type.IsPrimitive, Type.IsArray, Type.IsSubclassOf()), boxed the �yte, and allocated two strings. For a dotnet new maui -sc app the CoreCLR Debug typemap contains 3,630,293 blob bytes across three blobs, so this ran ~3.6 million times. dotnet-trace of an incremental build with a XAML-only change showed WriteStringBlobArray() at 2,507ms of the 3,612ms spent in <GenerateTypeMappings/>, with RuntimeType.IsPrimitiveImpl() alone accounting for 2,433ms. The rendering of a byte is one of only 256 possible strings, so cache them in a static table. Output is byte-for-byte identical. <GenerateTypeMappings/>: 3,727ms -> 568ms Total incremental build: 8.37s -> 3.11s Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Follow up to the string blob optimization: instead of special-casing byte arrays only, add a fast path at the top of `WriteType()` for the scalar types in `basicTypeMap` (`bool`, `byte`, `int`, ...). These types can never be a `StructureInstance`, an array, an `LlvmIrSectionedArrayBase` or an `LlvmIrStringBlob`, so all the reflection-heavy probing (`Type.IsPrimitive`, `Type.IsArray`, `Type.IsSubclassOf`) that ran before reaching the trivial `basicTypeMap` lookup was pure overhead. Arrays and string blobs write one element at a time, so this ran ~3.6 million times for a hello world MAUI application. This now benefits every caller, not just string blobs. Also replace the 256 entry lookup table added previously with a direct write of `i8 u0x` plus the two hex digits, and factor the hex digit conversion duplicated in `Files.cs` and `ScannerHashingHelper.cs` into a new shared `HexUtilities` class. Timings for an incremental, XAML only change to a `dotnet new maui -sc` project: * `GenerateTypeMappings`: 3,727ms -> ~600ms * Total build: 8.37s -> ~3.1s `typemaps.arm64-v8a.ll` is byte for byte identical before and after. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Drop the one line private ToHexString wrapper and have both callers use the shared helper directly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Covers upper/lower case digits, the default casing, empty input, all 256 byte values, and lengths either side of the 128 char stackalloc threshold in ToHexString(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Move the nibble splitting out of the callers and into two WriteHex() overloads, one writing to a Span<char> and one to a TextWriter. ToHexString() now shares the Span<char> overload, and LlvmIrGenerator no longer does its own bit math. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Rather than source linking HexUtilities.cs into the test project, grant it access via InternalsVisibleTo, matching Xamarin.Android.Build.Tasks and Microsoft.Android.Sdk.TrimmableTypeMap. The test assembly is now signed with product.snk, as the other test projects already are. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Write the two digits straight to the TextWriter rather than staging them through a 2 char stackalloc. Also fixes a stray brace introduced in the previous commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Removes the InternalsVisibleTo (and the product.snk signing it forced on the test project), plus the <Compile Include/> source link in Xamarin.Android.Build.Tasks, which references BaseTasks already. Microsoft.Android.Sdk.TrimmableTypeMap still links the source, because a ProjectReference to BaseTasks would drag Microsoft.Build.*, LibZipSharp, K4os.LZ4 and Mono.Unix into it. That copy stays \internal\ -- two public Microsoft.Android.Build.Tasks.HexUtilities types make Xamarin.Android.Build.Tasks fail with CS0433, since it references both assemblies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
There was a problem hiding this comment.
Pull request overview
This PR reduces <GenerateTypeMappings/> time by removing per-byte reflection/boxing overhead in LlvmIrGenerator and consolidating duplicated “bytes → hex” logic into a single shared HexUtilities helper.
Changes:
- Optimized string-blob IR emission by writing
i8 u0xNNdirectly for each byte and adding a basic-type fast path inWriteType(). - Introduced
HexUtilitiesinMicrosoft.Android.Build.BaseTasksand switched existing hex call sites (Files,ScannerHashingHelper) to use it. - Added dedicated unit tests for
HexUtilities.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Build.BaseTasks-Tests/HexUtilitiesTests.cs | Adds coverage for GetHexValue, WriteHex, and ToHexString behaviors (upper/lowercase, boundaries). |
| src/Xamarin.Android.Build.Tasks/Utilities/LlvmIrGenerator/LlvmIrGenerator.cs | Removes hot-path overhead by emitting byte constants directly and short-circuiting primitive type handling. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/ScannerHashingHelper.cs | Replaces local hex formatting with shared HexUtilities (lowercase). |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj | Source-links the shared HexUtilities.cs file into the TrimmableTypeMap project. |
| src/Microsoft.Android.Build.BaseTasks/Microsoft.Android.Build.BaseTasks.csproj | Defines a compile constant to make HexUtilities public only in BaseTasks. |
| src/Microsoft.Android.Build.BaseTasks/HexUtilities.cs | New shared implementation for allocation-free hex rendering. |
| src/Microsoft.Android.Build.BaseTasks/Files.cs | Replaces in-file hex conversion with HexUtilities.ToHexString(). |
Suppressed comments (3)
src/Microsoft.Android.Build.BaseTasks/HexUtilities.cs:67
- 🤖
⚠️ warning Formatting — Mono style requires a space before[in array/stackalloc expressions. This avoids mixing standard C# formatting with the repo’s convention.
Span<char> chars = charLength <= MaxStackCharLength
? stackalloc char[charLength]
: new char[charLength];
src/Microsoft.Android.Build.BaseTasks/HexUtilities.cs:41
- 🤖 ❌ error API design —
WriteHex(Span<char> destination, ...)assumesdestination.Length >= 2and otherwise throwsIndexOutOfRangeException. Since this helper ispublicin BaseTasks, validate the span length and throw anArgumentExceptionwith a clear message.
public static void WriteHex (Span<char> destination, byte value, bool upperCase = true)
{
destination [0] = GetHexValue (value >> 4, upperCase);
destination [1] = GetHexValue (value & 0x0f, upperCase);
}
src/Microsoft.Android.Build.BaseTasks/HexUtilities.cs:30
- 🤖
⚠️ warning Formatting — This repo uses Mono style spacing; casts should be written with a space (e.g.(char) (value + '0')) for consistency with the rest of BaseTasks.
if (value < 10)
return (char)(value + '0');
return (char)(value - 10 + (upperCase ? 'A' : 'a'));
Addresses review feedback: WriteHex(Span<char>) now throws ArgumentException instead of IndexOutOfRangeException when the destination is too short, and casts/array creation use the repo's spacing convention. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
|
Went through the review feedback -- checked each item against the actual repo rather than taking it at face value. Fixes in 195cab0. Applied
Both spacing issues came from copying the Tests: 126 -> 129 passing. Declined
|
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code Review — ⚠️ minor suggestions
Strong, well-scoped PR. I read the changed files in full and independently verified the key claim: the byte-write fast path and WriteType() basic-type short-circuit are behavior-preserving.
Verification
WriteByteTypeAndValue: originalWriteType()+WriteValue()emittedi8+u0x+{b:x02}(2 lowercase hex). New code emits"i8 u0x"+HexUtilities.WriteHex(..., upperCase: false)— byte-for-byte identical. It's only ever reached fromWriteStringBlobArray(), which forcesNumberFormat = HexadecimalandelementType = byte, so the hardcoded values are safe today.WriteType()fast path: nothing inbasicTypeMapcan be a structure/array/sectioned-array/string-blob, and the constructedLlvmTypeInfomatches the generalGetIRType()path (lines 535–542) exactly. Equivalent output, less reflection.HexUtilitiesunification: casing preserved per caller —Files.ToHexStringstays uppercase (defaultupperCase: true);ScannerHashingHelperand the generator passupperCase: false. The#if MICROSOFT_ANDROID_BUILD_BASETASKSpublic/internal split correctly avoidsCS0433inXamarin.Android.Build.Tasks.- Tests: 22 new tests cover casing, the 2-char span contract, null
TextWriter, all 256 byte values, empty input, and the 128-char stackalloc threshold — thorough.
CI
GitHub-surfaced Azure DevOps checks are green (Java.Interop + Android Tools tests passing); Linux/Mac/Windows build legs still in progress at review time. No failures observed.
Suggestions (non-blocking, posted inline)
- 💡 Guard the hidden
i8/hex invariant inWriteByteTypeAndValuewith aDebug.Assert. - 💡 Now-
publicGetHexValuesilently produces garbage for out-of-range input — consider a Debug-only assert.
Nice work, especially the measured-and-rejected notes and byte-identical .ll SHA verification. 👍
Generated by Android PR Reviewer for #12279 · 124.2 AIC · ⌖ 18.7 AIC · ⊞ 6.9K
Comment /review to run again
HexUtilities is now public, so validate the
Debug.Assert is already [Conditional ("DEBUG")], so the wrapper added nothing
-- the call and its interpolated string are elided in release either way.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
Stacked on #12279 — review that first; this PR's diff is one file, `LlvmIrGenerator.cs`. `WriteStringBlobArray()` emitted one LLVM IR element per byte of the type map string blobs: ```llvm @type_map_managed_type_names = ... constant [3035518 x i8] [ ; 'Android.OS.AsyncTask' @ 0 i8 u0x41, i8 u0x6e, ... ] ``` For a hello world MAUI app that's ~3.6 million elements and a 52.6MB `typemaps.arm64-v8a.ll`, which is slow to write and much slower for `llc` to consume. Emit a single constant byte string instead — the same form the generator already uses for constant string literals: ```llvm @type_map_managed_type_names = ... constant [3035518 x i8] c"Android.OS.AsyncTask\00..." ``` Bytes stream through a pooled `char []` rather than millions of single-character `TextWriter` writes. ## Results `dotnet new maui -sc`, Debug, clean build, `-nodeReuse:false`. The machine is shared and noisy, so raw per-run values are shown rather than a single median: | Metric | Before | After | | --- | --- | --- | | `typemaps.arm64-v8a.ll` | 52.6MB | **15.2MB** | | `CompileNativeAssembly` (`llc`) | 2239, 2287, 2177ms | **224, 168, 203, 201, 221, 203ms** | | `GenerateTypeMappings` | 1489, 1649, 1414ms | 645, 1187, 1206, 543, 576, 643ms | `CompileNativeAssembly` is the headline: ~10x faster, with no overlap between the before and after ranges. That directly addresses the `CompileNativeAssembly` regression seen in the CoreCLR-vs-Mono binlog comparison that motivated this work. `GenerateTypeMappings` is genuinely faster — every "after" run beats every "before" run — but it's noisy enough (543-1206ms across identical binaries) that I won't quote a percentage. `LinkApplicationSharedLibraries` and total build time are **unchanged**. The linker consumes a byte-identical `.o`, so there is nothing there to win. ## Correctness `typemaps.arm64-v8a.o` is **byte-for-byte identical** before and after (SHA256 `73E4A982E8C8098FB7909EA550CDEE392E19AAB6938E145A4C362BE0F0417DCF`, 4161824 bytes). One caveat for anyone reproducing this: the `-ci.<branch>.<n>` SDK version string is embedded in the assembly names blob and changes on every SDK rebuild, so `.o` hashes only compare within a matched before/after pair. Additional checks: * All three globals decode from the `.ll` to exactly their declared `[N x i8]` length — `type_map_assembly_names` 2707, `type_map_managed_type_names` 3035518, `type_map_java_type_names` 592060 — and the decoded bytes appear verbatim in the `.o`. * All 28,225 `i32` offsets in the file (16,567 java + 11,658 managed) resolve correctly against the decoded blobs, zero unresolved. This matters because those offsets index into the blobs. * `Microsoft.Android.Build.BaseTasks-Tests` 129/129, `LlvmIrGeneratorTests` 9/9, `GenerateNativeApplicationConfigSourcesTests` 4/4. The last of those runs `llc` on a generated `.ll` and parses the resulting `.s`, so it round-trips a string blob end to end. Nothing parses blob contents out of the `.ll`, so no test changes were needed. `TrimmableTypeMapBuildTests` only matches the array *type* (`[N x`), which is unchanged. ## Tradeoffs * **Per-string `; 'Foo' @ 123` comments are gone.** A constant string literal has to fit on a single line and `;` comments extend to the end of the line, so they can't be interleaved. They were also a large fraction of the file size. * **The managed type names line is ~3MB wide.** `llc` handles it fine; text editors and diff tools will not enjoy it. * **15.2MB, not ~5MB.** The remaining bulk is the `i32` offset/index arrays, which this change doesn't touch — the obvious next target. * This removes the last caller of `HexUtilities.WriteHex (TextWriter, ...)` that #12279 added for the old hot path. The overload is public and unit-tested, so it's left in place. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes a hot spot in
LlvmIrGeneratorthat dominates<GenerateTypeMappings/>, and unifies three copies of "bytes → hex" while we're in there.The problem
WriteStringBlobArray()emits one LLVM IR element per byte of the type-map string blobs — 3,630,285 calls for a hello-world MAUI app (96.5% of all elements in the 52.6 MB.ll). Each one went throughWriteType()+WriteValue(), which doType.IsPrimitive,TypeUtilities.IsArray(),IsSubclassOf(), plus boxing and several string allocations — all to emit the constanti8and two hex digits.The fix
"i8 u0x"+ two hex chars. No reflection, no allocation.WriteType()—basicTypeMap.ContainsKey()short-circuits the reflection probing. Safe because nothing inbasicTypeMapcan be a structure instance, array, sectioned array or string blob. This helps all callers, including the 132,549i32struct fields.Results
dotnet new maui -sc, CoreCLR,GenerateTypeMappingson a clean build:~4× faster, −2.4 s. Measured by reverting only
LlvmIrGenerator.csagainst an otherwise identical SDK build, so nothing else varied.Output is byte-identical:
typemaps.arm64-v8a.llSHA256D875D690…across every run, before and after.HexUtilities.csWe had three private hex implementations. They're now one
public static class HexUtilitiesinMicrosoft.Android.Build.BaseTasks:Files.GetHexValue()/Files.ToHexString()HexUtilities.ToHexString()ScannerHashingHelper.ToHexString()/GetHexValue()HexUtilities.ToHexString (hash, upperCase: false)HexUtilities.WriteHex (TextWriter, byte, …)API:
GetHexValue(),WriteHex(Span<char>, …),WriteHex(TextWriter, …)(nostackalloc),ToHexString(). TheupperCaseflag exists becauseFilesemits uppercase whileScannerHashingHelperand LLVM IR emit lowercase.Microsoft.Android.Sdk.TrimmableTypeMapstill source-links the file — aProjectReferenceto BaseTasks would drag inMicrosoft.Build.*, LibZipSharp (native assets), K4os.LZ4 and Mono.Unix. That linked copy staysinternalvia one#if, since two publicMicrosoft.Android.Build.Tasks.HexUtilitiestypes makeXamarin.Android.Build.Tasksfail withCS0433(it references both assemblies).Measured and rejected
Short-circuiting all numeric writes saved ~33 ms of ~1,500 ms — noise. Not worth an
i32fast path; the rest is mostly I/O for a 52.6 MB file.The real remaining win is emitting
c"..."string literals instead of per-bytei8arrays (~52.6 MB → ~5 MB), which would also cutllctime. Left for a follow-up.Testing
Microsoft.Android.Build.BaseTasks-Tests: 129/129 (was 107 — 22 newHexUtilitiestests)LlvmIrGeneratorTests: 9/9Xamarin.Android.slnbuilds clean