Cache CSETensorHash's raw_data hash by tensor_id(), the actual bottleneck - #10
Open
take-cheeze wants to merge 3 commits into
Open
Conversation
Previously, eliminate_duplicate_initializer and eliminate_common_ subexpression each cleared onnxoptimizer's TensorContentDigest cache (tensor_content_hash.h) at their own entry, because it was keyed by Tensor* -- only safe within one pass call, since a freed tensor's address can be reused by an unrelated, differently-contented tensor shortly after (Graph::eraseInitializer, an attribute being replaced). That forced re-hashing every retained tensor's content on every one of these two passes' calls, even when nothing about that tensor had changed -- the dominant cost on deep repeated-block models whose OptAndShape fixed point (onnxsim) can take dozens of rounds to converge (onnxsim issue #633). Depends on onnxsim/onnx@debe14bb (Tensor::tensor_id()): a per-object id minted fresh on every construction and assignment, so it can never alias two different tensors' content the way a reused address could. Re-keying the cache on tensor_id() instead of &tensor removes that constraint structurally, so: - eliminate_duplicate_initializer/eliminate_common_subexpression no longer clear the cache themselves. - Optimizer::optimize(Graph&, ...) (and the OptimizeGraph/ OptimizeGraphFixed free functions wrapping it) gained a clear_tensor_digest_cache parameter, defaulting to true: it clears the cache at entry, bounding its memory to one optimize() call -- the same safety envelope every existing caller already had, just applied once per call instead of once per pass invocation within it (itself already a real, if smaller, win for everyone). - A caller that holds one Graph across several optimize() calls (e.g. onnxsim's OptAndShape, which now clears the cache itself once at the top of its own fixed point and passes clear_tensor_digest_cache=false to every OptimizeGraphFixed call inside it) can opt out to keep digests warm across all of them instead. Bumps third_party/onnx to the corresponding tensor_id() commit, cherry-picked onto this repo's own (diverged) third_party/onnx lineage as onnxsim/onnx@c3b180f3, matching the existing pattern for this repo's onnxsim-specific patches. Signed-off-by: Claude <noreply@anthropic.com>
…neck eliminate_duplicate_initializer/eliminate_common_subexpression's hash-map lookups had two hashing paths: TensorContentDigest (typed-field tensors, already cached across rounds since the previous commit) and CSETensorHash's raw_data branch (a std::hash<std::string> scan of the tensor's raw bytes, uncached). Profiling onnxsim issue #633's mixer_l16_224_in21k repro showed raw_data is overwhelmingly the exercised path on real exported models (15929 raw_data hash calls vs 1193 typed-field), and that hashing -- not comparison, which was already negligible at 0.13ms total -- accounted for 73% of the entire Optimize() phase: eliminate_duplicate_initializer alone was ~10.4s of a ~14.1s Optimize() call, ~10.3s of which was raw_data hashing. 98%+ of those hash calls were recomputing a value already seen earlier in the same run. Adds g_raw_hash_cache (cse_util.h), keyed by Tensor::tensor_id() for the same freed-pointer-reuse safety as TensorContentDigest's cache: a Tensor's content is never mutated in place by any onnx-optimizer pass (only replaced wholesale, which mints a fresh tensor_id() and simply misses this cache), so a hash computed for a tensor that survives unchanged across rounds stays valid. Threaded through Optimizer::optimize(Graph&, ...)'s existing clear_tensor_digest_cache parameter alongside ClearTensorContentDigestCache, so callers that already opt out of clearing one cache per round (e.g. onnxsim's OptAndShape) get both. Also lands the pass-phase profiling instrumentation used to find this (pass.h/pass.cc/pass_manager.cc's PassPhaseTiming/PassTotalTiming, cse_util.h's CSEHashCompareTiming) -- off by default (SetPassPhaseProfilingEnabled), zero overhead when disabled, gated behind onnxsim's ONNXSIM_PROFILE_PASS_PHASES env var. Kept as a permanent, documented diagnostic since it was instrumental in finding this and the existing TensorContentDigest caching was, on its own, ineffective (it only covered the rare typed-field path). Measured on mixer_l16_224_in21k_Opset17 (833MB, the flagship #633 repro), same host, before/after this commit: eliminate_duplicate_initializer 10446ms -> 648ms, Optimize() phase 14200ms -> 4343ms, end-to-end wall time 46.68s -> 37.5s (~20% faster), 93.23% cache hit rate (14850/15929 calls). Output node count and check_ok unchanged (582 nodes, ok=True). cait_xxs36 (where this pass isn't dominant) unaffected: 1758->1558 nodes, ok=True, ~23s, matching its pre-change baseline. Signed-off-by: Claude <noreply@anthropic.com>
Member
Author
|
The Generated by Claude Code Generated by Claude Code |
…nals Follow-up profiling for onnxsim issue #633: after the raw_hash_cache fix, eliminate_common_subexpression and eliminate_deadend became the two heaviest passes in Optimize() (1399ms and 1310ms of ~4507ms total on mixer_l16_224_in21k_Opset17, vs eliminate_duplicate_initializer's now- reduced 661ms). Neither cost is explained by CSETensorHash/CSETensorCompare (589.5ms/0.06ms total, mostly attributed to eliminate_duplicate_initializer), so this adds instrumentation to find out what actually dominates each: - cse_util.h: CSEHashCompareTiming gains node_hash_*/node_equal_* fields. CSENodeHash::operator() and CSEEqual::operator() are now timed as a whole, plus their attributeNames()+std::sort sub-step is timed separately (attributeNames() returns std::vector<Symbol> by value -- an allocation -- on every call; this isolates whether that allocation is the actual cost). - pass.h/pass.cc: new CSEPassTiming (filter/lookup/replace phases of EliminateCommonSubexpressions's own loop) and DeadendPassTiming (hasUses()/destroyCurrent() phases of EliminateDead's loop), same on/off toggle as the existing pass-phase timers. - eliminate_common_subexpression.h / eliminate_deadend.h: wire up the above at their respective call sites. Off by default, zero std::chrono overhead when disabled.
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.
Motivation and Context
Follow-up to onnxsim/onnxsim#633 and onnxsim/onnx#2 (this PR's prerequisite:
Tensor::tensor_id()).eliminate_duplicate_initializer/eliminate_common_subexpression's hash-map lookups go through two paths depending on whether an initializer israw_data(real exported models, almost always) or typed-field (rare). The typed-field path already had a cross-round cache (TensorContentDigest, this branch's own earlier commitd4664a9); the raw_data path — the one that actually matters for real models — did not. Every initializer's raw bytes were rescanned from scratch (std::hash<std::string>overtensor->raw()) on every one ofOptAndShape's ~50 rounds, even for initializers unchanged since round 1.Profiling
mixer_l16_224_in21k_Opset17(833MB, issue #633's flagship repro) showed this was 73% of the entireOptimize()phase:eliminate_duplicate_initializeralone was ~10.4s of a ~14.1sOptimize()call, ~10.3s of which was raw_data hashing — with 98%+ of those hash calls recomputing a value already seen earlier in the same run. Comparison (the memcmp fast path) was already negligible (0.13ms total) — this was purely a hashing problem, not a comparison problem.Changes
cse_util.h: newg_raw_hash_cache, keyed byTensor::tensor_id()for the same freed-pointer-reuse safetyTensorContentDigest's cache already relies on — aTensor's content is never mutated in place by any onnx-optimizer pass (only replaced wholesale, which mints a freshtensor_id()and simply misses the cache).CSETensorHash's raw_data branch now checks this cache before rescanning bytes.optimize.h:Optimizer::optimize(Graph&, ...)'s existingclear_tensor_digest_cacheparameter now also clears the new cache alongsideTensorContentDigest's, so callers that already opt out of clearing one cache per round (e.g. onnxsim'sOptAndShape) get both automatically.pass.h/pass.cc/pass_manager.cc: pass-phase profiling instrumentation used to find this (PassPhaseTiming,PassTotalTiming,SetPassPhaseProfilingEnabled) — off by default, zerostd::chronooverhead when disabled. Kept as a permanent, documented diagnostic since it was instrumental here and the existingTensorContentDigestcaching alone was, on its own, ineffective (it only covered the rare typed-field path — this PR is the one that actually matters).Testing
Measured, same host, before/after this commit (
mixer_l16_224_in21k_Opset17, via onnxsim's full rebuild):eliminate_duplicate_initializerOptimize()phaseOutput unchanged: 582 nodes,
ok=True.cait_xxs36_224_Opset17(where this pass isn't dominant) unaffected: 1758→1558 nodes,ok=True, ~23s, matching its pre-change baseline.Full validation via onnxsim: core pytest suite (53/53),
test_python_api.py(48 passed/1 deselected, includingtest_exprimental_simplify_subgraph), both green.Generated by Claude Code