Conversation
Two const read paths wrote node state without synchronisation, so a shared expression could not be read from two threads. hash_value() now publishes through a small state machine: one thread computes, others wait for the value rather than reading it half-written. Derived assumptions move from a std::set into two atomic words - every fact is an empty tag, so a whole set is a bitmask - which removes the tree a concurrent reader could corrupt and keeps the epoch stamp that decides staleness. Asserted facts stay authoritative and still bump the epoch; a snapshot is published in one store, so a reader sees the previous set or the new one. Signed-off-by: petlenz <[email protected]>
The rebase onto #508 met its review fixes: derived facts stamped with an epoch, stale ones discarded on read, scratch copies that do not claim a node, and the tensor manager bumping the epoch so det's positivity follows its operand. effective() reads the mask through the same staleness gate. Signed-off-by: petlenz <[email protected]>
petlenz
force-pushed
the
fix-504-shared-node-races
branch
from
September 20, 2026 20:12
b5d1b8a to
9ed96c2
Compare
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
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.
Closes #504. Stacks on #508 (
fix-490-491-assumption-consistency) and builds on its epoch stamp rather than replacing it.Two
const-looking read paths mutated shared node state, so a shared expression could not be read from two threads.Mechanisms, and the measurements that chose them
Lazy hash — compute-once publication.
hash_value()runs a three-state machine (unset → computing → ready): the winner of a CAS computes, everyone else waits forreadyrather than reading a half-written value. A throw during computation resets tounsetso a waiter takes over instead of spinning forever. Alternatives measured on a 400-node chain, Release, mean of two runs:The cold path costs one CAS per node (≈14 ns), so first-hash roughly doubles; the warm path is unchanged (one acquire load) and construction got faster, because the assumption change below drops a
std::setper node. Net build-then-hash is a wash: 36.3 µs → 36.1 µs. Eager hashing at construction was rejected — it pays the cost for every node whether or not it is ever hashed, and #415 wants laziness kept.Derived assumptions — bitmask in two atomic words. Every
numeric_assumptionalternative is an empty tag, so a fact set is a bitmask.facts_holds asserted/intrinsic facts,derived_holds(epoch << 16) | maskpublished in one store. A reader sees the previous snapshot or the new one, never a partially rebuilt container. This deletes thestd::_Rb_treecorruption outright rather than guarding it; a per-node mutex was the alternative and costs more memory per node than the whole manager now uses. #508's epoch semantics are preserved exactly: asserted facts bump the epoch and clear the derived word,replace_derivedwipes prior derived facts the wayset_ = facts.set_did, andset_inferred()still pins intrinsic facts so they never go stale.invalidate_hash()now clears the publication state too — otherwise a mutated n-ary node kept areadyflag over a zeroed hash. That is what three existing tests (MutatedCopyDropsStaleCachedHash,SubCancelInvalidatesAndCollapses,PythagoreanBranchHygiene) caught when the first draft missed it.Thread-safety contract this implies
assume()/.assumption()/clear()on a symbol, or the construction-timeinvalidate_hash()/insert_hash()paths. Same rule as a standard container: concurrent readers are fine, a concurrent writer is not. The epoch is atomic, so an assertion racing a reader yields the old or new fact set, but the surrounding set-up is still a user-side race.Verification
TSan, gcc-14, ASLR disabled via
setarch -R(TSan otherwise aborts with "unexpected memory mapping" on this kernel — an environment issue, not the code; test discovery also had to move toPRE_TESTso the build-time run does not trip it).expression::hash_value(), 5 in the assumption path includingstd::_Rb_treeinsert/rebalance andnumeric_assumption_manager::{inferred,stale}.--warnings-as-errors='*') clean.ThreadSafetyTest.hadds five tests. The two hash tests and the assumption test reproduce the races; the assumption one deliberately builds its reference from a separate tree so the shared one reaches the threads with its facts still underived — an earlier draft computed the reference first, which pre-filled the cache and silently stopped reproducing the race. The tensor-query and per-thread-evaluator tests are lock-ins: they pass on the unfixed base too, since tensor annotations are computed at construction.Assertions compare against a single-threaded reference rather than pinning a particular inference result, so the test does not depend on how strong the assumption rules happen to be.