[CoreCLR] Replace xxhash with crc32#12013
Conversation
Replace CoreCLR LLVM typemap xxHash lookups with CRC32 hashes from System.IO.Compression.Native. Keep collision-safe lookup by comparing the original managed or Java type name after matching the hash. This is a first step toward removing the CoreCLR xxHash dependency to reduce size while preserving runtime lookup performance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Emit CoreCLR assembly stores with 32-bit CRC32 name hashes and update the CoreCLR native assembly-store lookup to use the shared CRC32 helper. Keep MonoVM assembly stores on the existing xxHash layout, bump the CoreCLR store format to v4, and update the mk2 store reader to infer index entry width from the header index size. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
Replace CoreCLR runtime property, DSO APK, and DSO/AOT cache xxHash lookups with CRC32 hashes from the shared native CRC32 helper. Update generated CoreCLR app config, desktop app stub data, and strict generated-layout test parsing for the smaller 32-bit hash fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch the CoreCLR dynamic p/invoke override and generated find_pinvoke tables to 32-bit CRC32 hashes. Keep the precompiled p/invoke table on the existing xxHash layout while removing xxHash from the shared p/invoke override header and fallback cache path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
Switch the CoreCLR precompiled p/invoke override table and generator to 32-bit CRC32 hashes. This removes the remaining CoreCLR p/invoke override dependency on xxHash while keeping the table collision check in the generator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the common Search helper generic-only and move the xxHash-specific overloads to a MonoVM-only search-xxhash.hh wrapper. This prevents CoreCLR native code from including shared/xxhash.hh through the common search helper while preserving MonoVM's existing hash_t search callsites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop adding external/xxHash and constexpr-xxh3 include paths to CoreCLR native targets now that CoreCLR no longer includes shared/xxhash.hh. Keep the xxHash include support available for MonoVM and NativeAOT paths that still use it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduce a CoreCLR-local hash_t alias to uint32_t in the CRC32 helper and use it for CoreCLR hash fields, lookup signatures, and generated p/invoke tables. This keeps the code readable without reintroducing the shared xxHash wrapper or changing MonoVM's hash_t layout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a compile-time crc32_hash overload for string literals and use it in CoreCLR generated/default tables instead of hardcoded hex hash values. This keeps the generated p/invoke table and app stub self-documenting while preserving the runtime CRC32 helper for dynamic strings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allow generated-environment tests to parse decimal uint32 values above Int32.MaxValue and update CoreCLR APK size baselines after the CRC32 native size changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the CoreCLR debug typemap string fallback now that CRC32 lookups verify matching strings, and remove the unused CoreCLR AOT DSO cache path while failing fast if a CoreCLR config tries to use it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reuse the runtime property arrays passed to coreclr_initialize for the host runtime contract callback instead of emitting a duplicate CRC32-indexed lookup table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leave the host runtime contract in place for p/invoke and assembly probing, but set get_runtime_property to null because Android passes runtime configuration through coreclr_initialize and the callback only produced negative host-property probes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unused CoreCLR application config structure-info fields that were only kept for potential future host configuration data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
simonrozsival
added
the
ready-to-review
There was a problem hiding this comment.
Pull request overview
This PR removes CoreCLR’s native dependency on xxHash/constexpr-xxh3 by switching multiple CoreCLR lookup paths (typemaps, assembly store indices, DSO cache, and p/invoke override) to 32-bit CRC32, while keeping MonoVM on the existing xxHash-based layouts. It also bumps the CoreCLR assembly-store format to v4 and updates associated tooling/tests.
Changes:
- Replace CoreCLR xxHash lookups with CRC32 across typemap, assembly store, DSO cache, and p/invoke override paths.
- Update CoreCLR assembly-store format to v4 (including variable index entry sizes) and adjust readers/parsers accordingly.
- Isolate xxHash-specific search helpers to MonoVM and remove CoreCLR’s xxHash include paths/dependencies.
Show a summary per file
| File | Description |
|---|---|
| tools/assembly-store-reader-mk2/AssemblyStore/StoreReader_V2.cs | Accept CoreCLR v4 store versions and support variable index entry sizes when reading. |
| tools/assembly-store-reader-mk2/AssemblyStore/StoreReader_V2.Classes.cs | Add explicit 32/64-bit index entry size constants for parsing. |
| src/Xamarin.Android.Build.Tasks/Utilities/TypeMappingReleaseNativeAssemblyGeneratorCLR.cs | Emit CoreCLR typemap data using CRC32 + collision-safe secondary string comparisons. |
| src/Xamarin.Android.Build.Tasks/Utilities/TypeMappingDebugNativeAssemblyGeneratorCLR.cs | Update debug typemap layout to CRC32 + module UUID array and simplify hash usage. |
| src/Xamarin.Android.Build.Tasks/Utilities/TypeMapHelper.cs | Introduce CRC32 helpers for CoreCLR hashing (names/bytes). |
| src/Xamarin.Android.Build.Tasks/Utilities/PreservePinvokesNativeAssemblyGenerator.cs | Switch preserved p/invoke hashing to CRC32 and simplify 32/64-bit handling. |
| src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreGenerator.cs | Generate CoreCLR v4 assembly store indices using CRC32 (32-bit) and adjust index parsing. |
| src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreBuilder.cs | Pass target runtime to store generator to select CRC32 vs xxHash. |
| src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGeneratorCLR.cs | Convert CoreCLR DSO cache hashes to CRC32 and remove CoreCLR-only dead AOT/runtime-property paths. |
| src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigCLR.cs | Reflect CoreCLR ApplicationConfig layout change (remove AOT cache entry count). |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateEmptyTypemapStub.cs | Update stub LLVM IR struct layouts for CoreCLR typemap changes. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.CoreCLR.apkdesc | Update expected APK description sizes after native/layout changes. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.CoreCLR.apkdesc | Update expected APK description sizes after native/layout changes. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs | Update layout parsing/constants for CoreCLR config + CRC32-sized fields. |
| src/native/mono/runtime-base/search-xxhash.hh | New: MonoVM-only xxHash search wrapper to keep CoreCLR free of xxHash includes. |
| src/native/mono/runtime-base/monodroid-dl.hh | Move MonoVM search usage to SearchXxHash wrapper. |
| src/native/mono/monodroid/mono-image-loader.hh | Use SearchXxHash for MonoVM image cache lookups. |
| src/native/mono/monodroid/embedded-assemblies.cc | Use SearchXxHash for MonoVM assembly store + typemap lookups. |
| src/native/common/include/runtime-base/search.hh | Refactor search helpers: introduce lower_bound and rework binary_search templates. |
| src/native/CMakeLists.txt | Remove CoreCLR include paths for external xxHash/constexpr-xxh3. |
| src/native/clr/xamarin-app-stub/application_dso_stub.cc | Update CoreCLR stub to CRC32 and remove dead runtime-property/AOT-cache paths. |
| src/native/clr/shared/CMakeLists.txt | Remove EXTERNAL include path from CoreCLR shared target options. |
| src/native/clr/pinvoke-override/precompiled.cc | Switch precompiled p/invoke override hashing to CRC32 and adjust slow-path call. |
| src/native/clr/pinvoke-override/pinvoke-tables.include | Regenerate internal p/invoke tables to use CRC32 hashes and constexpr crc32_hash calls. |
| src/native/clr/pinvoke-override/generate-pinvoke-tables.cc | Update generator to emit CRC32-based tables and update constexpr validation. |
| src/native/clr/pinvoke-override/dynamic.cc | Switch dynamic p/invoke override hashing to CRC32 and adjust slow-path call. |
| src/native/clr/include/xamarin-app.hh | Bump CoreCLR store format to v4 and update public struct layouts for CRC32-based fields. |
| src/native/clr/include/runtime-base/monodroid-dl.hh | Remove CoreCLR AOT cache path and switch DSO lookups to CRC32. |
| src/native/clr/include/runtime-base/crc32.hh | New: constexpr CRC32 implementation for CoreCLR native hashing (compile-time + runtime). |
| src/native/clr/include/host/typemap.hh | Update typemap lookup APIs to support collision-safe matching (hash + string). |
| src/native/clr/include/host/pinvoke-override.hh | Switch map hashing to CRC32 and simplify APIs (remove explicit hash parameters). |
| src/native/clr/include/host/pinvoke-override-impl.hh | Update override implementation to match new map APIs and loader calls. |
| src/native/clr/include/host/host.hh | Remove unused runtime-property host callback wiring for CoreCLR. |
| src/native/clr/host/typemap.cc | Implement CRC32 hashing and collision-safe typemap matching via lower_bound + string compare. |
| src/native/clr/host/host.cc | Switch CoreCLR DSO APK hashing to CRC32 and remove unused runtime-property callback implementation. |
| src/native/clr/host/CMakeLists.txt | Remove EXTERNAL include path from CoreCLR host target options. |
| src/native/clr/host/assembly-store.cc | Switch CoreCLR assembly-store name hashing to CRC32. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/Xamarin.Android.Build.Tasks/Tasks/GenerateEmptyTypemapStub.cs:75
typemap_use_hasheswas removed from the CoreCLR typemap implementation, but the debug stub LLVM IR still declares it. Keeping stale stub symbols makes the stub diverge from the real layout and can hide future regressions in layout-validation tests.
@type_map = dso_local constant %struct.TypeMap zeroinitializer, align 8
@typemap_use_hashes = dso_local constant i8 1, align 1
@type_map_managed_type_info = dso_local constant [0 x %struct.TypeMapManagedTypeInfo] zeroinitializer, align 8
- Files reviewed: 37/37 changed files
- Comments generated: 4
- Avoid per-call byte[] allocation in HashNameForCLR via stackalloc - Replace unused switch expression with explicit arch guard - Validate index_size divisibility in GetIndexEntrySize (both readers) - Remove stale typemap_use_hashes symbol from debug typemap stub Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 13027389-e819-4a27-b98f-dbcd7586a1ab
…eclr-typemap-crc32 # Conflicts: # src/native/clr/pinvoke-override/dynamic.cc
|
/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.
Review: [CoreCLR] Replace xxhash with crc32
Overall this is a clean, well-scoped refactor. I independently verified the core correctness invariant and the collision-safety story, and found one issue — in a test helper.
🔎 Verified correct
- C#/native CRC32 agreement (non-empty):
System.IO.Hashing.Crc32.HashToUInt32(build) matches the hand-rolledcrc32_hash(runtime) — both are CRC-32/ISO-HDLC (poly0xEDB88320). Confirmed numerically:"test"→0xd87f7e0c,"java-interop"→0x33b98009, matching the nativestatic_asserts. - Empty-input handling is consistent:
HashNameForCLR/HashBytesForCLRand nativecrc32_hashboth special-case empty →0xFFFFFFFF, so they agree (they intentionally diverge from raw CRC32's0). - Collision safety with 32-bit hashes:
- typemap lookups now do
lower_bound+ linear scan with an actual string compare (same_string) after the hash match — safe against CRC32 collisions. - pinvoke table generation hard-fails the build on collision (
generate-pinvoke-tables.ccreturns1), so the pure-hashfind_pinvoke_addressis safe. - assembly store / DSO cache keep pure-hash binary search, but 32-bit hashing already applied to 32-bit ABIs before this PR and the entry counts are small — consistent with the documented trade-off.
- typemap lookups now do
- Struct layout:
AssemblyStoreIndexEntryis[[gnu::packed]], so the 32-bithash_tgives a 9-byte on-disk entry that matches the mk2 reader'sNativeSize32and the generator's packed writes. - Assembly store v4 + mk2 reader: reader derives entry size from
index_size / index_entry_count(with a divisibility guard) and validates it againstNativeSize32/64— correctly handles the new CoreCLR 32-bit-on-64-bit case. - Removal of dead CoreCLR paths (AOT DSO cache,
get_runtime_property, debug typemap string fallback) looks self-consistent, with a guard added if alibaot-DSO unexpectedly appears in CoreCLR config.
⚠️ Issue (see inline comment)
EnvironmentHelper.TryParseInteger(test helper): theInt32/Int64fallback usesNumberStyles.None, which rejects a leading-. That makes the signed fallback dead code and drops negative decimals thatllccan emit — a regression from the priorNumberStyles.Integerbehavior, and it contradicts the method's own updated comment. Fix: useNumberStyles.AllowLeadingSign.
CI
At review time: Java.Interop tests (Windows/Mac) and CLA are green; the Linux/Mac/Windows build+test legs are still in progress with no failures yet.
Automated review — surfacing observations only, not a blocking gate.
Generated by Android PR Reviewer for #12013 · 412.7 AIC · ⌖ 23.7 AIC · ⊞ 6.8K
Comment /review to run again
Generated uint32_t and uint64_t values are emitted using unsigned decimal representations, so remove the unnecessary signed parsing fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 11570f57-4494-4f92-977b-47f37db2e81b
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code Review — [CoreCLR] Replace xxhash with crc32
Overall this is a clean, internally-consistent change. CI is green across the sampled Azure DevOps checks. The migration from xxHash/constexpr-xxh3 to CRC32 (reusing System.IO.Compression.Native) is well executed, the search.hh refactor to lower_bound primitives is tidy, and removing the AOT DSO cache / runtime-property host callback / CI_TYPEMAP_DEBUG_USE_STRINGS dead paths is a nice cleanup. The store format bump (v3→v4) and the matching EnvironmentHelper test updates (field count 20→19, DSOCacheEntry size 32→24, unsigned parsing) line up correctly with the struct changes.
Strongest positive: the typemap lookups were made genuinely collision-safe — after matching the 32-bit hash you now compare the actual strings via the new same_string() helper, and the C# generators sort by hash then StringComparer.Ordinal so grouped collisions stay consistent. Native crc32_hash (reflected poly 0xedb88320, init/final 0xffffffff) matches System.IO.Hashing.Crc32, and pinvoke-tables.include now emits crc32_hash("...") consteval calls so build/runtime hashes can't drift.
Main thing to consider (see inline): the same collision-safety was not applied to the assembly-store index, the DSO cache, or the DSO apk lookups — those still match on 32-bit hash only, with no runtime name verification and no build-time duplicate-hash detection. Since CoreCLR was previously 64-bit-only here, this is a real (if small) reduction in collision resistance for those tables. Internal p/invoke tables are fine — generate-pinvoke-tables.cc already fails the build on collisions. Worth either adding a cheap build-time collision check for the store index or documenting the intentional trade-off.
Minor: a one-line comment on the crc32_hash empty-string → UINT32_MAX sentinel (mirrored in TypeMapHelper) would protect against a future well-meaning "fix" to 0.
Nothing here looks like a blocker — the inline notes are a safety consideration and a doc nit.
Generated by Android PR Reviewer for #12013 · 259.6 AIC · ⌖ 19 AIC · ⊞ 6.8K
Comment /review to run again
| return storePath; | ||
| } | ||
|
|
||
| static ulong HashAssemblyName (byte[] assemblyNameBytes, bool useCrc32NameHashes, bool use64BitNameHashes) |
There was a problem hiding this comment.
🤖 HashBytesForCLR) instead of the previous 64-bit xxHash, and the native lookups that consume it — AssemblyStore::find_assembly_store_entry, MonodroidDl::find_dso_cache_entry, and find_dso_apk_entry — still match on hash equality only, with no secondary name comparison (unlike the typemap paths, which you correctly made collision-safe via same_string). Because CoreCLR is 64-bit-only, this is a genuine reduction in collision resistance (2^32 vs 2^64) for these tables, and a CRC32 collision between two assembly (or DSO) names in the same app would silently resolve to the wrong entry — a hard-to-diagnose failure with no build-time signal, since WriteIndex here only sorts and never checks for duplicate hashes.
Consider adding a build-time collision check for the store index (and ideally the DSO/apk tables) — it's cheap and deterministic — or storing the name/real_name_hash for a runtime tie-break like the typemap now does. At minimum, please document in the commit message that these CoreCLR lookups intentionally trade 64→32-bit hash width without name verification.
(Rule: Reinventing/weakening lookup safety; collision handling)
| constexpr auto crc32_hash (const char *value, size_t len) noexcept -> hash_t | ||
| { | ||
| if (len == 0) [[unlikely]] { | ||
| return std::numeric_limits<uint32_t>::max (); |
There was a problem hiding this comment.
🤖 💡 Documentation — crc32_hash returns UINT32_MAX for an empty input rather than the standard CRC32 of the empty string (0). This is a deliberate sentinel that must stay in lockstep with the managed side (TypeMapHelper.HashNameForCLR/HashBytesForCLR, which also special-case length 0 to UInt32.MaxValue) — and it does today. A one-line comment here noting why empty maps to MAX (and that the C# generators mirror it) would save a future reader from "fixing" it to 0 and silently desyncing the build-time and runtime hashes.
(Rule: Docs describe intent not reality)
Goal
Remove the CoreCLR native dependency on xxHash/constexpr-xxh3 in .NET for Android lookup paths while preserving runtime behavior and startup performance. The primary improvement is dependency removal; app size and startup time are expected to be unchanged within measurement noise.
Summary
Benchmark
CoreCLR MAUI Release/R2R sample, Android arm64, Samsung SM-A165F, interleaved runs, no reboot. The comparison below uses #12010 as the xxHash baseline because it contains the p/invoke resolver cleanup needed to run with the current runtime packs; this isolates the CRC32/dependency-removal changes in this PR.
Size
Startup, 30x mixed cold + hot, IQR outliers removed
Result: app size and startup times are unchanged within measurement noise. The meaningful win is removing CoreCLR's dependency on the xxHash/constexpr-xxh3 native dependency chain.
Validation