[LlvmIrGenerator] ~4x faster type map generation - #12279
[LlvmIrGenerator] ~4x faster type map generation#12279jonathanpeppers wants to merge 11 commits into
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
Copilot-Session: d1a93958-51c0-4df6-b848-97dc8c93f3e6
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