Skip to content

[native] Reuse native compression for CRC32 hashes #13356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
simonrozsival wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[native] Reuse native compression for CRC32 hashes #13356

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 15 additions & 21 deletions src/native/clr/include/runtime-base/crc32.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,41 +5,35 @@
#include <limits>
#include <string_view>

extern "C" uint32_t CompressionNative_Crc32 (uint32_t crc, uint8_t *buffer, int32_t len) noexcept;

namespace xamarin::android {
using hash_t = uint32_t;
static constexpr hash_t CRC32_POLYNOMIAL = 0xedb88320;

[[gnu::always_inline]]
constexpr auto crc32_hash (const char *value, size_t len) noexcept -> hash_t
inline auto crc32_hash (const char *value, size_t len) noexcept -> hash_t
{
// An empty input deliberately maps to UINT32_MAX rather than to the standard CRC32 of the
// empty string (0). This is a sentinel that must stay in lockstep with the managed generators
// (AssemblyStoreGenerator and CoreClrBootstrapBuilder, which likewise special-case a zero-length
// input to UInt32.MaxValue); the build-time and runtime hashes must agree, so do not "fix"
// this to 0.
// Preserve the empty-input sentinel rather than returning the standard empty CRC32 (0).
if (len == 0) [[unlikely]] {
return std::numeric_limits<uint32_t>::max ();
}

hash_t crc = 0xffffffff;
for (size_t i = 0; i < len; i++) {
crc ^= static_cast<uint8_t>(value [i]);
for (size_t bit = 0; bit < 8; bit++) {
crc = (crc >> 1) ^ ((crc & 1) != 0 ? CRC32_POLYNOMIAL : 0);
}
// The native compression ABI takes a mutable pointer but does not modify the bytes.
auto bytes = const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(value));
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]] {

@github-actions github-actions Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 💡 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}

crc = CompressionNative_Crc32 (crc, bytes, MaxChunkSize);
bytes += MaxChunkSize;
len -= MaxChunkSize;
}

return ~crc;
}

template<size_t Size>
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));

@github-actions github-actions Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 💡 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}

}

[[gnu::always_inline]]
constexpr auto crc32_hash (std::string_view const& value) noexcept -> hash_t
inline auto crc32_hash (std::string_view const& value) noexcept -> hash_t
{
return crc32_hash (value.data (), value.length ());
}
Expand Down
Loading