Repository navigation
[native] Reuse native compression for CRC32 hashes - #13356
simonrozsival wants to merge 1 commit into
Conversation
Context: #12013 CoreCLR assembly and DSO lookups use a hand-written IEEE CRC32 implementation, while the host already links libSystem.IO.Compression.Native for Zstandard decompression. The pinned compression library exports CompressionNative_Crc32 on all three supported CoreCLR ABIs, so reuse that implementation without adding a library dependency. Preserve the UINT32_MAX empty-input sentinel and process size_t lengths in INT32_MAX-sized chunks to match the compression entry point's signed length parameter. Start the CRC with zero because the compression implementation applies the initial and final complements internally. Remove the unused internal constant-evaluation overload. A fixed-seed host probe compared the original implementation with both production wrapper overloads for 50 random byte buffers: all matched. The empty sentinel and independent IEEE CRC32 check vector also passed. This used the installed macOS .NET 11 RC2 compression library. API-24 probes compiled and linked against the pinned Android .NET 12 libraries on ARM, ARM64 and x64 without libc++. Android execution, full product validation and size/performance measurements remain pending. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Assembly and DSO lookup now cross a native runtime ABI, while hash parity, collision handling, and full SDK/device validation against the pinned Android runtime remain pending.
No open issues 🎉
What changed in this PR
This PR replaces CoreCLR’s hand-written CRC32 implementation with the existing native compression library’s CompressionNative_Crc32 export, building on the CRC32 lookup migration in #12013 without adding a direct zlib dependency.
Changes:
- Preserves the
UINT32_MAXempty-input sentinel and uses a zero seed for nonempty inputs. - Splits large inputs into chunks compatible with the export’s signed 32-bit length parameter.
- Removes the unused constant-evaluation overload while leaving managed hash generation and collision handling unchanged.
| File | Description |
|---|---|
src/native/clr/include/runtime-base/crc32.hh |
Replaces manual CRC32 computation with native compression calls, preserving empty-input behavior and supporting large inputs. |
Review effort: Balanced
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
simonrozsival
added
the
ready-to-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review: no blocking findings
0 errors · 0 warnings · 2 inline suggestions. Reviewed the complete changed header, both hashing callers, host linkage, and managed hash producers before reconciling the PR description.
The change preserves the empty-input sentinel, starts CRC accumulation at zero, and bounds each native call to a signed 32-bit length while carrying the previous CRC forward. Both current callers hash runtime names, so removing the unused constant-evaluation overload does not remove a current compile-time use. The CoreCLR host already links System.IO.Compression.Native; no new link dependency is introduced. I found no correctness issue in the diff. The two optional suggestions are posted inline.
CI verification is limited: all 42 GitHub check runs report completed/success, including the aggregate dotnet-android check for Azure build 1656223. Direct Azure build/timeline verification was blocked by CLI authentication/tool failure, and access to the public endpoint was denied. I therefore cannot independently confirm the Azure build result or give an unconditional CI-green/merge-ready verdict. The local/device results in the description were not rerun during this review.
Generated by Android PR Reviewer for #13356 · copilot · gpt61 · 21.1 AIC · ⌖ 4.35 AIC · ⊞ 20K
Comment /review to run again
| constexpr int32_t MaxChunkSize = std::numeric_limits<int32_t>::max (); | ||
| // IEEE CRC32 (0xedb88320); the compression shim applies the initial/final complements. | ||
| hash_t crc = 0; | ||
| while (len > MaxChunkSize) [[unlikely]] { |
There was a problem hiding this comment.
🤖 💡 Testing — Could we retain the small correctness/recording-shim probes described in the PR as checked-in regression coverage? This introduces a native ABI dependency and seed chaining, but the only changed file is the implementation. A deterministic test covering "123456789" → 0xcbf43926, empty/default views → UINT32_MAX, embedded NUL/high-bit bytes, and recorded calls around INT32_MAX would preserve the evidence for future runtime-pack updates. The recording shim can exercise chunk lengths, pointer advancement, and seed propagation without allocating multi-gigabyte buffers; at least one vector should also exercise the real compression export.
{Rule: Test edge cases and preserve regression coverage}
| consteval auto crc32_hash (const char (&value)[Size]) noexcept -> hash_t | ||
| { | ||
| return crc32_hash (value, Size - 1); | ||
| return CompressionNative_Crc32 (crc, bytes, static_cast<int32_t>(len)); |
There was a problem hiding this comment.
🤖 💡 Performance — Consider adding a matched ARM64 measurement for representative short assembly/DSO names before treating the runtime cost as settled. Both callers use this on lookup paths, so replacing the inlined bit loop with an external call trades per-byte work for call/PLT overhead; large-buffer CRC throughput would not establish the effect here. The PR correctly says this is unmeasured, so this is a request for evidence, not a claim that the change regresses performance. A short-name microbenchmark plus repeated app-start measurements would make the maintainability tradeoff easier to assess.
{Rule: Consider startup-time impact}
Context: #12013
CoreCLR assembly and DSO lookups maintain a hand-written IEEE CRC32 implementation even though the host already links
libSystem.IO.Compression.Nativefor Zstandard decompression. Reuse itsCompressionNative_Crc32export rather than adding a direct zlib dependency.Removing
constexprdoes not remove any compile-time hashing used by the current runtime. The only two consumers,AssemblyStore::open_assembly()andMonodroidDl::monodroid_dlopen(), hash names supplied at runtime. The internal literal-arrayconstevaloverload has no callers, and this header is not a shipped public API. The change removes the unused ability to use this helper in constant expressions; the runtime wrapper remains inline. Separately, replacing the inlined bit loop with an external compression-library call changes runtime execution, so its effect on short-name lookup performance still needs measurement.Keep the
UINT32_MAXempty-input sentinel and split large inputs intoINT32_MAX-sized chunks to match the native entry point's signed length parameter. Seed with zero: the compression implementation already performs IEEE CRC32's initial/final complements; no length XOR is required. Managed generation and collision disambiguation are unchanged. The final change removes six net source lines; the maintainability benefit is eliminating the duplicate algorithm, not a large source reduction.Validation was performed locally;
$artifactsbelow is the out-of-tree probe/evidence directory, not checked-in tests:make prepare CONFIGURATION=Release— passed.make all CONFIGURATION=Releasebuilt the native/product solution but failed local-workload setup because the extra API37.1 reference assembly was absent.make leeroy CONFIGURATION=Releasethen passed, building extra API levels and configuring the local workload../dotnet-local.sh test bin/TestRelease/net11.0/Microsoft.Android.Build.Tasks.Tests.dll --filter 'FullyQualifiedName~CreateAssemblyStoreTests|FullyQualifiedName~CoreClrBootstrapBlobTests|FullyQualifiedName~GenerateNativeApplicationConfigSourcesTests' --logger 'trx;LogFileName=crc32-host-tests.trx' --results-directory "$artifacts/host-test-results"— 19 passed, zero failed."$ndk/bin/${triple}24-clang++" -std=c++23 -O2 -Wall -Wextra -Werror -fno-exceptions -fno-rtti -nostdlib++ -I src/native/clr/include -I "$artifacts" "$artifacts/crc32-correctness.cc" "$library" -o "$artifacts/crc32-correctness-android-$rid"— passed for ARM, ARM64 and x64 at API24 using NDK28.2.13676358 and each ABI's exact pinned12.0.0-alpha.1.26523.103compression library; ELF inspection confirmed the expected CRC export/import and no libc++ dependency. ARM32/x64 execution was not performed.adb -s emulator-5554 shell 'cd /data/local/tmp/crc32-eval-767749ff && sha256sum libSystem.IO.Compression.Native.so && LD_LIBRARY_PATH=. ./crc32-correctness-android-arm64 && ./crc32-chunking-android-arm64'— passed on ARM64/API35: 54,115 original/candidate comparisons through both overloads, IEEE known vectors, empty/null/default-view sentinels, and seven recording-shim chunk boundaries through2*INT32_MAX+17. The chunk probe checks length splitting/seed chaining, not real multi-gigabyte data processing. All 50 fixed-seed Android random inputs also matched exact managedSystem.IO.Hashing10.0.12 results../dotnet-local.sh build "$artifacts/collision-app/CrcCollision.csproj" -c Release -t:Rebuild, followed byadb -s emulator-5554 install -r "$artifacts/matched/$side.apk"andadb -s emulator-5554 shell am start -n com.example.crc32eval767749ff/scrc64d32e1997ab86caac.MainActivity— both matched baseline/candidate APKs passed actual assembly-store and DSO collision lookup, repeated reverse-order lookups, and missing-DSO handling. Distinct assemblies/native libraries use the genuinely colliding namesxtazkbhaltigandqzxwhiivqwxpand return distinct values. The probe explicitly pins the Microsoft.NETCore.App framework reference to12.0.0-alpha.1.26523.103; SDK defaults otherwise select an older .NET11 runtime../dotnet-local.sh build -t:Install -c Release -p:RuntimeIdentifier=android-arm64 -p:UseMonoRuntime=false -p:CustomAfterMicrosoftCommonTargets="$artifacts/pin-test-runtime.targets" tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj, then from that project directory../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:RuntimeIdentifier=android-arm64 -p:UseMonoRuntime=false -p:CustomAfterMicrosoftCommonTargets="$artifacts/pin-test-runtime.targets" --report-trx --results-directory "$artifacts/runtime-test-results-pinned"— 991 passed, seven skipped, zero failed on ARM64/API35 with the explicit pinned runtime override. APK extraction verified the exact pinned compression library and locally built candidate host bytes.Matched Release native rebuilds used the same configured CMake directories/toolchain/flags with only this header changed:
ninja -C src/native/obj/Release/CoreCLR/android-$abi-Release net-android.release. Candidate rebuilds reproduced the original candidate binaries byte-for-byte. Product ELF sizes fell by 4/88/72 bytes for ARM/ARM64/x64 respectively;llvm-strip --strip-allcopies fell by 108/88/120 bytes. In the matched ARM64 APKs, every ZIP entry's content exceptlibmonodroid.sowas identical. That runtime entry shrank by 88 raw bytes and 124 compressed bytes, but both signed APKs were 6,431,898 bytes: no final APK-size reduction after alignment.DT_NEEDEDis unchanged for all three ABIs: no new library or libc++ dependency. Compression already depends on platformlibz.so. One CRC import/PLT entry is added. Both product APKs contain the exact pinned compression SHA2562d9c71cf57e9b8ffa11a3bb8e5ef200aece4f6a385ba20d03124e8e7e60ee607; candidatelibmonodroid.soSHA256 is5a047ca45056627b011249d415609970b8be353ab0b1eddce6c1cceda4fffb93. The observed compression/libz/host file-backed mapping extents were unchanged; this is not RSS/PSS evidence.Earlier macOS ARM64 checks against installed .NET11 RC2 also passed 50/50 random inputs, but are separate from the pinned Android execution above. Short-name lookup/startup performance remains unmeasured; no no-regression or performance improvement is claimed. No timing was performed under the correctness-only validation lease, which has been released.