cse_util.h: bound CSETensorHash's raw_data hash cost for large tensors - #15
Open
take-cheeze wants to merge 4 commits into
Open
Conversation
std::hash<std::string> over the full raw_data buffer was a genuine one-time-per-initializer cost on raw_data-heavy models with large tensors (unlike g_raw_hash_cache misses, this isn't something caching can amortize away -- see onnxsim issue #633 follow-up profiling, where it dominated eliminate_duplicate_initializer's time on several models). HashRawDataBounded hashes small tensors exactly as before, and for larger ones hashes size + up to 256 evenly-spaced 64-byte windows instead of every byte, bounding the cost to a small constant regardless of tensor size. This only degrades CSETensorHash's bucketing quality -- CSETensorCompare's full raw() == raw() memcmp is still the sole source of truth for equality on every hash-bucket hit, so a spurious collision can only cost one extra (already-cheap) memcmp, never an incorrect merge.
Deliberately constructs two large raw_data tensors that differ only in a byte outside HashRawDataBounded's sampled windows (a real, expected hash collision under the new bounded hash) and confirms eliminate_ duplicate_initializer still keeps both -- CSETensorCompare's full memcmp, not the hash, is what decides equality. Also checks a genuinely identical large pair still gets deduped.
w3/w4 previously reused the same bytes as w1 ('base'), so
eliminate_duplicate_initializer correctly folded all three into one
survivor (w1) and the count(w3)+count(w4)==1 assertion was wrong, not
the pass. Give w3/w4 their own distinct fill byte so they form an
independent duplicate pair.
…sors Mirrors the raw_data fix (HashRawDataBounded): CSETensorHash's typed-field branch previously hashed via std::hash<std::string>(TensorContentDigest(...)), paying a full BLAKE3 pass over every parsed element even though the hash is only ever used as a bucketing key (CSETensorCompare / TensorContentDigest equality remain the sole source of truth on any hash-bucket hit). Model- regression CI's pass-phase bottleneck summary showed this dominating eliminate_duplicate_initializer's cost on typed-field-heavy legacy-opset models once the raw_data path was fixed. Adds HashTypedFieldBounded, sharing HashBytesBounded with the raw_data path and preserving the signed-zero canonicalization (CanonicalizeZero) that ComputeTensorContentDigest applies for FLOAT/DOUBLE/COMPLEX64/COMPLEX128. Adds EliminateDuplicateInitializerLargeTypedField, adversarially verified to catch an incorrect merge when hash-bucket membership is mistakenly trusted as equality. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H4AHXs8oJU1UNbjs8LoQWu
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to the model-regression per-pass profiling that found
extract_constant_to_initializer(fixed in #14) — with that bottleneck gone,eliminate_duplicate_initializer'sCSETensorHashraw_data hash became the next-largest one-time cost on raw_data-heavy models with large tensors (e.g. vgg19).CSETensorHash's raw_data path hashed the entire buffer viastd::hash<std::string>on every cache miss. Unlikeg_raw_hash_cachehits, a miss isn't a caching gap that can be amortized away — it's a genuine one-time-per-initializer cost, so the only lever left was making that one-time hash itself cheaper.Changes
HashRawDataBounded: hashes tensors ≤4KB exactly as before; for larger ones, hashes size + up to 256 evenly-spaced 64-byte windows instead of every byte, bounding the cost to a small constant regardless of tensor size (5KB or 500MB).CSETensorHash's raw_data branch in place of the full-bufferstd::hash<std::string>call.Why this is safe:
CSETensorHashonly needs to be a good bucketing key.CSETensorCompare's raw_data fast path — a fulllhs->raw() == rhs->raw()memcmp — is the sole source of truth for equality on every hash-bucket hit, regardless of how the hash was computed. A lower-quality/sampled hash can never cause an incorrect merge; the only cost of a spurious collision is one extra, already-cheap memcmp against a candidate that turns out not to match.Test plan
EliminateDuplicateInitializerLargeRawData(tests/test_simple.cc): deliberately constructs two 100KB tensors that differ only in a byte outside the sampled windows (a real, expected hash collision under the new bounded hash) and confirmseliminate_duplicate_initializerstill keeps both — proving the hash-collision-safety invariant above, not just asserting it in a comment. Also checks a genuinely identical large pair still gets deduped to one.test_simple(optimizer's own gtest suite): 5/5 passing, including the new test.tests/, minus torch/timm-dependent files not installed in this sandbox): 428 passed, 0 failed.Note on base branch
This targets
claude/extract-constant-move-fix4uz(PR #14's branch) rather thanmain, since it depends ong_raw_hash_cache/CSETensorHashmachinery that currently only exists on that unmerged stack (introduced in #10). Not independently mergeable tomainuntil that stack lands.Generated by Claude Code