Add Tensor::tensor_id(): a per-object id safe to cache on, unlike &tensor - #2
Open
take-cheeze wants to merge 4 commits into
Open
Conversation
…nsor Gives every Tensor a process-wide-unique uint64_t id, freshly minted by every constructor and every assignment operator -- never preserved across a copy, a move, or a reassignment. Motivation: onnx-optimizer's TensorContentDigest cache (onnxoptimizer/passes/tensor_content_hash.h) memoizes a BLAKE3 digest per tensor to avoid re-hashing the same content on every hash-bucket lookup and equality check. It currently keys that cache off `&tensor`, which is only safe within a single pass call: a `Tensor*` can be freed (e.g. via Graph::eraseInitializer, or a Node attribute being replaced) and its memory reused by an unrelated, differently-contented tensor shortly after -- silently aliasing a stale digest onto the new tensor if the cache lived any longer than that. That's exactly why the cache is cleared at the entry of every eliminate_duplicate_initializer/ eliminate_common_subexpression call today, which in turn means a tensor's digest gets recomputed on every one of the dozens of rounds a deep repeated-block model's OptAndShape fixed point can take (onnxsim issue onnx#633) even though the tensor itself hasn't changed. tensor_id() removes that constraint structurally instead of by convention: since a fresh id is minted on every construction and assignment, two live Tensor objects (or an old, freed one and a new one reusing its address) can never share an id while their contents could differ. A cache keyed on tensor_id() can safely outlive a single pass call -- see the onnxoptimizer follow-up commit that puts this to use. Requires user-declared copy/move constructors and assignment operators (previously all four were implicit): tensor_id_ must be excluded from the copy, and refreshed on assignment, which the compiler-generated versions can't express. Every other field is copied/moved verbatim, so behavior is otherwise unchanged. Tested: onnx_gtests (142/143 passed, 1 pre-existing locale-dependent skip) and the full Python test suite (6244 passed, 0 failures) both green against this change. Signed-off-by: Claude <noreply@anthropic.com>
cppcoreguidelines-special-member-functions flagged Tensor for declaring copy/move ctor and assignment without a destructor; add an explicit defaulted one to complete the rule-of-five declaration. cppcoreguidelines-rvalue-reference-param-not-moved flagged MoveFieldsFrom(Tensor&&) even though every field is moved from other -- the check only recognizes std::move(other) on the parameter itself, not std::move(other.member) on its fields, so this is a false positive; silenced with NOLINTNEXTLINE.
Member
Author
|
Two CI notes:
Generated by Claude Code Generated by Claude Code |
Value::uses()/setUniqueName() (and anything else built on Graph::forEachNode()) search for kCaptured nodes in nested subgraphs via forSelfAndEachSubGraphImpl, which -- for every node in the graph -- called attributeNames() (an allocating std::vector<Symbol> return by value) just to check whether any attribute has kind g or gs. Profiling onnxsim issue onnx#633's flagship repro (mixer_l16_224_in21k_ Opset17, a model with zero control-flow subgraphs) showed this made Value::uses() effectively O(graph size) per call: onnx-optimizer's eliminate_common_subexpression and eliminate_deadend each call Node::hasUses() (-> Value::uses()) once per node while sweeping the whole graph, so both passes became O(N^2) in node count. Measured ~1.25s/1.24s of wall time in each pass attributable to this, on a graph with no subgraphs to ever find. Adds Attributes<Derived>::hasSubgraphAttribute(): a non-allocating equivalent of "does attributeNames() contain a g/gs symbol", since values_ already stores each attribute's kind directly. Used as a cheap pre-check in forSelfAndEachSubGraphImpl before the allocating attributeNames() enumeration, which now only runs for the rare node that actually has a nested subgraph (Loop/If/Scan). Behavior is unchanged -- this only skips work that attributeNames()'s own loop would have found nothing from.
std::string data{istreambuf_iterator{stream}, istreambuf_iterator{}}
copies a file one character at a time (each increment pays a stream
buffer-boundary check), instead of one bulk read. For small models this
is noise; for a large one it is seconds -- measured costing the bulk of
a ~16s gap between onnx-optimizer's path-based loadModel/saveModel and
the equivalent Python-side onnx.load on an 833MB model (onnxsim issue
onnx#633's investigation into loadModel's path-based entry points, used by
its own SimplifyPath fast path).
Sizes the file up front via std::filesystem::file_size and does a single
read() into a pre-sized string; falls back to the old iterator-based read
if the size can't be determined (e.g. a pipe). Correctness is checked via
gcount() rather than the stream's good()/eof() flags, since a read() that
consumes exactly to EOF can set eofbit on a stream implementation even
though every requested byte was read.
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 #1 (this repo). #1 made
Graph::initializers_avector<unique_ptr<Tensor>>so aTensor's address survives other initializers being inserted/erased around it — but that alone isn't enough to safely key a cache on&tensoracross multiple onnx-optimizer pass calls: a freedTensor*can still be reused by an unrelated, differently-contented tensor (Graph::eraseInitializer, a Node attribute being replaced), silently aliasing a stale cache entry onto new content.Change
Adds
Tensor::tensor_id(): a per-object id minted fresh by every constructor and every assignment operator (copy or move), never preserved across a copy, a move, or a reassignment. This means two liveTensorobjects — or an old, freed one and a new one reusing its address — can never share an id while their contents could differ, so a cache keyed ontensor_id()can only ever miss on address reuse, never silently alias.Implementation: an explicit copy/move constructor and copy/move assignment operator (previously all implicit/defaulted), each either minting a fresh id via the field's default member initializer (construction) or explicitly reassigning one (assignment) — every other field is copied/moved member-wise unchanged.
Testing
Added
TensorTest.TensorId*unit tests (tests/cpp/tensor_test.cc) covering: distinct ids across default/copy/move construction, id refreshed by copy/move assignment, self-assignment is a no-op, and 1000 default constructions all get distinct ids.Full validation: onnx's own C++ gtest suite (142/143 passing, 1 pre-existing unrelated locale skip) and Python test suite (6244 passed) both green against this exact commit.
Consumer
This is groundwork for onnxsim/optimizer#10 (also part of this same follow-up), which uses
tensor_id()to cacheCSETensorHash's raw_data hash acrossOptAndShaperounds — measured as a real, substantial speedup on issue onnx#633's flagship repro model. See that PR for the end-to-end numbers.Generated by Claude Code