[LlvmIrGenerator] emit c"..." literals for string blobs - #12280
[LlvmIrGenerator] emit c"..." literals for string blobs#12280jonathanpeppers wants to merge 5 commits into
Conversation
`WriteStringBlobArray()` emitted one LLVM IR element per byte of the type map
string blobs:
@type_map_managed_type_names = ... constant [3035518 x i8] [
; 'Android.OS.AsyncTask' @ 0
i8 u0x41, i8 u0x6e, ...
]
For a hello world MAUI app that is ~3.6 million elements, producing a 52.6MB
`typemaps.arm64-v8a.ll`, which is slow both to write and for `llc` to consume.
Emit a single constant byte string instead, which is the same form the
generator already uses for constant string literals:
@type_map_managed_type_names = ... constant [3035518 x i8] c"Android.OS.AsyncTask\00..."
Bytes are streamed through a pooled `char[]` chunk buffer to avoid millions of
single-character `TextWriter` writes.
A constant string literal has to fit on a single line, and `;` comments extend
to the end of the line, so the per-string `; 'Foo' @ 123` comments are gone.
Results for a hello world MAUI app (`dotnet new maui -sc`, Debug, median of 3):
* `typemaps.arm64-v8a.ll`: 52.6MB -> 15.2MB
* `GenerateTypeMappings`: 1489ms -> 1187ms
* `CompileNativeAssembly`: 2239ms -> 203ms
The resulting `typemaps.arm64-v8a.o` is byte-for-byte identical to the one
produced before this change.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6a038cc-e09a-455d-8f08-1ea17ecfd849
ArrayPool<T>.Rent() may hand back an array larger than requested. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6a038cc-e09a-455d-8f08-1ea17ecfd849
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6a038cc-e09a-455d-8f08-1ea17ecfd849
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6a038cc-e09a-455d-8f08-1ea17ecfd849
There was a problem hiding this comment.
Pull request overview
This PR updates LlvmIrGenerator to emit type-map string blobs as LLVM IR constant byte strings (c"...") instead of per-byte i8 u0xNN array elements, dramatically reducing .ll size and improving llc compile time for large type maps.
Changes:
- Reworked
WriteStringBlobArray()to stream a singlec"..."literal for the blob, escaping bytes as needed. - Switched the hot output path to use a pooled
char[]chunk buffer to avoid millions of tinyTextWriterwrites. - Removed per-string inline comments in blob emission (no longer representable within a single-line constant string literal).
Makes the `exactly two hex digits` contract obvious at the call site. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6a038cc-e09a-455d-8f08-1ea17ecfd849
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM
Solid, well-scoped performance change to WriteStringBlobArray. Emitting a single c"..." byte-string constant instead of millions of i8 u0xNN array elements is the right approach, and the reasoning is clearly documented in the method comment.
Correctness review (independent):
- LLVM IR escaping is correct:
",\, and everything outside printable ASCII[32,127)are hex-escaped;127(DEL) and0(NUL) fall through to the escape path as expected. - Buffer bounds are safe: the literal path flushes at
chunkUsed == chunkCapacity(needs 1 char) and the escape path atchunkUsed + 3 > chunkCapacity(needs\+ 2 hex). AfterFlush()the 4096-char rented buffer always has room, so no overflow ofchunk.AsSpan(chunkUsed, 2). - Per-string terminating NUL is preserved (
WriteByte(0)), so total byte count still matches the declared[N x i8]length. ArrayPool<char>rent/return is correctly wrapped intry/finally, and usingchunk.Lengthas the capacity handlesRentover-allocation.- No test impact:
EnvironmentHelper.ReadStringBlobparses thellc-produced.ascizoutput, which is downstream of the.lland byte-identical per the PR's.ohash verification.
Verification: The PR body's byte-identical .o (SHA256 match) plus offset-resolution and unit-test checks are exactly the right evidence for a change like this — thank you for the thoroughness.
Two non-blocking 💡 suggestions inline (magic-number naming; uppercase-hex cosmetic note). Neither blocks merge.
CI note: at review time the pipeline shows pending with no completed checks — confirm the dotnet-android build/tests go green before merging.
Generated by Android PR Reviewer for #12280 · 91.4 AIC · ⌖ 18.7 AIC · ⊞ 6.9K
Comment /review to run again
| WriteCommaWithStride (counter); | ||
| } else { | ||
| context.Output.Write (context.CurrentIndent); | ||
| if (chunkUsed + 3 > chunkCapacity) { |
There was a problem hiding this comment.
🤖 💡 Code organization — The + 3 here is the maximum width of a single emitted escape (\ + two hex digits), and 4096 on line 919 is the buffer size. Both are magic numbers whose relationship is implicit. Consider a named const int MaxEscapeWidth = 3; (and a const int ChunkSize = 4096;) so the flush-threshold intent is self-documenting.
(non-blocking maintainability nit)
| WriteByteTypeAndValue (0); // Terminating NUL is counted for each string, but not included in its bytes | ||
|
|
||
| chunk [chunkUsed++] = '\\'; | ||
| HexUtilities.WriteHex (chunk.AsSpan (chunkUsed, 2), b, upperCase: true); |
There was a problem hiding this comment.
🤖 💡 Documentation — Nice catch reusing the span-based WriteHex overload here for the zero-allocation hot path. Note this switches the emitted hex to uppercase (\FF) where the old array form used lowercase (u0xff); that's cosmetic-only for the .ll and llc accepts both, so no action needed — just flagging it since it's the one visible textual difference a reader diffing .ll files might notice.
(informational; no change required)
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: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 forllcto consume.Emit a single constant byte string instead — the same form the generator already uses for constant string literals:
Bytes stream through a pooled
char []rather than millions of single-characterTextWriterwrites.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:typemaps.arm64-v8a.llCompileNativeAssembly(llc)GenerateTypeMappingsCompileNativeAssemblyis the headline: ~10x faster, with no overlap between the before and after ranges. That directly addresses theCompileNativeAssemblyregression seen in the CoreCLR-vs-Mono binlog comparison that motivated this work.GenerateTypeMappingsis 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.LinkApplicationSharedLibrariesand total build time are unchanged. The linker consumes a byte-identical.o, so there is nothing there to win.Correctness
typemaps.arm64-v8a.ois byte-for-byte identical before and after (SHA25673E4A982E8C8098FB7909EA550CDEE392E19AAB6938E145A4C362BE0F0417DCF, 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.ohashes only compare within a matched before/after pair.Additional checks:
.llto exactly their declared[N x i8]length —type_map_assembly_names2707,type_map_managed_type_names3035518,type_map_java_type_names592060 — and the decoded bytes appear verbatim in the.o.i32offsets 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-Tests129/129,LlvmIrGeneratorTests9/9,GenerateNativeApplicationConfigSourcesTests4/4. The last of those runsllcon a generated.lland 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.TrimmableTypeMapBuildTestsonly matches the array type ([N x), which is unchanged.Tradeoffs
; 'Foo' @ 123comments 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.llchandles it fine; text editors and diff tools will not enjoy it.i32offset/index arrays, which this change doesn't touch — the obvious next target.HexUtilities.WriteHex (TextWriter, ...)that [LlvmIrGenerator] ~4x faster type map generation #12279 added for the old hot path. The overload is public and unit-tested, so it's left in place.Squash on merge — the 4 commits are kept separate for readability.