From 799d0d898d9d9c19ce198c050673877d27211775 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sat, 19 Sep 2026 15:28:10 +0200 Subject: [PATCH 1/2] Fix #504: publish the lazy hash and derived facts atomically 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 --- include/numsim_cas/core/assumptions.h | 134 ++++++++++++++++++-------- include/numsim_cas/core/expression.h | 27 +++++- include/numsim_cas/core/n_ary_tree.h | 2 +- src/numsim_cas/core/expression.cpp | 23 ++++- tests/CMakeLists.txt | 1 + tests/ThreadSafetyTest.h | 125 ++++++++++++++++++++++++ tests/main.cpp | 1 + 7 files changed, 263 insertions(+), 50 deletions(-) create mode 100644 tests/ThreadSafetyTest.h diff --git a/include/numsim_cas/core/assumptions.h b/include/numsim_cas/core/assumptions.h index ce8eae10..772997d0 100644 --- a/include/numsim_cas/core/assumptions.h +++ b/include/numsim_cas/core/assumptions.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -129,83 +130,94 @@ inline std::uint64_t current_assumption_epoch() noexcept { } } // namespace detail +// Every numeric_assumption alternative is an empty tag, so a whole fact set +// is a bitmask. Holding it in two atomic words (asserted facts, and the +// derived snapshot tagged with the epoch it was derived at) means a reader +// on a shared node sees a complete set, never a half-updated container. class numeric_assumption_manager { public: + using set_type = std::set; + numeric_assumption_manager() = default; // attached_ is a property of the manager's place, not of its contents: // copies are scratch until a node claims them. numeric_assumption_manager(numeric_assumption_manager const &o) - : set_(o.set_), inferred_(o.inferred_), epoch_(o.epoch_) {} + : facts_(o.facts_.load(std::memory_order_acquire)), + derived_(o.derived_.load(std::memory_order_acquire)), + intrinsic_(o.intrinsic_.load(std::memory_order_acquire)) {} numeric_assumption_manager(numeric_assumption_manager &&o) noexcept - : set_(std::move(o.set_)), inferred_(o.inferred_), epoch_(o.epoch_) {} + : facts_(o.facts_.load(std::memory_order_acquire)), + derived_(o.derived_.load(std::memory_order_acquire)), + intrinsic_(o.intrinsic_.load(std::memory_order_acquire)) {} numeric_assumption_manager &operator=(numeric_assumption_manager const &o) { - set_ = o.set_; - inferred_ = o.inferred_; - epoch_ = o.epoch_; + if (this != &o) + assign_from(o); return *this; } numeric_assumption_manager & operator=(numeric_assumption_manager &&o) noexcept { - set_ = std::move(o.set_); - inferred_ = o.inferred_; - epoch_ = o.epoch_; + assign_from(o); return *this; } - ~numeric_assumption_manager() = default; + // Asserted facts: they pin the node and tell every dependent to re-derive. void insert(numeric_assumption a) { - set_.insert(a); + facts_.fetch_or(bit_of(a), std::memory_order_acq_rel); + derived_.store(0, std::memory_order_release); invalidate_dependents(); } void erase(numeric_assumption const &a) { - set_.erase(a); + facts_.fetch_and(~bit_of(a), std::memory_order_acq_rel); + derived_.store(0, std::memory_order_release); + invalidate_dependents(); + } + void clear() { + facts_.store(0, std::memory_order_release); + derived_.store(0, std::memory_order_release); invalidate_dependents(); } // A fact derived before the last assertion is no longer believed. bool contains(numeric_assumption const &a) const { if (stale(detail::current_assumption_epoch())) return false; - return set_.find(a) != set_.end(); - } - void clear() { - set_.clear(); - invalidate_dependents(); - } - auto const &data() const { return set_; } - - // The facts as currently believed, detached from any node: stale derived - // ones are dropped rather than re-derived. Domains without a propagator - // read through this. - numeric_assumption_manager effective() const { - numeric_assumption_manager m; - if (!stale(detail::current_assumption_epoch())) - m.set_ = set_; - return m; + return (mask() & bit_of(a)) != 0; } + set_type data() const { return set_from_mask(mask()); } // Facts the library establishes itself: intrinsic to a constant or // computed from children. They invalidate nothing. - void insert_derived(numeric_assumption a) { set_.insert(a); } + void insert_derived(numeric_assumption a) { + facts_.fetch_or(bit_of(a), std::memory_order_acq_rel); + } + // Published as one word, so a concurrent reader sees either the previous + // snapshot or the new one. void replace_derived(numeric_assumption_manager const &facts, std::uint64_t epoch) { - set_ = facts.set_; - inferred_ = true; - epoch_ = epoch; + facts_.store(0, std::memory_order_release); + derived_.store((epoch << mask_width) | facts.mask(), + std::memory_order_release); } - // inferred(): the facts are established. Intrinsic ones (epoch 0) are - // never re-derived; facts stamped with an epoch go stale when it moves. - bool inferred() const noexcept { return inferred_; } + // inferred(): the facts are established. Intrinsic ones are never + // re-derived; derived ones go stale when the epoch moves on. + bool inferred() const noexcept { + return intrinsic_.load(std::memory_order_acquire) || + derived_.load(std::memory_order_acquire) != 0; + } void set_inferred() noexcept { - inferred_ = true; - epoch_ = 0; + intrinsic_.store(true, std::memory_order_release); + derived_.store(0, std::memory_order_release); } + // Facts computed from another node's annotation: they go stale with it. void set_inferred_at(std::uint64_t epoch) noexcept { - inferred_ = true; - epoch_ = epoch; + intrinsic_.store(false, std::memory_order_release); + derived_.store((epoch << mask_width) | mask(), std::memory_order_release); } bool stale(std::uint64_t now) const noexcept { - return epoch_ != 0 && epoch_ != now; + if (intrinsic_.load(std::memory_order_acquire)) + return false; + auto const w = derived_.load(std::memory_order_acquire); + return w != 0 && (w >> mask_width) != now; } // Only managers that live on a node invalidate dependents; scratch @@ -213,15 +225,53 @@ class numeric_assumption_manager { void attach_to_node() noexcept { attached_ = true; } private: + static constexpr unsigned mask_width = 16; + static constexpr std::uint64_t mask_bits = + (std::uint64_t{1} << mask_width) - 1; + static_assert(std::variant_size_v <= mask_width, + "a fact must fit in the published mask"); + + static std::uint64_t bit_of(numeric_assumption const &a) noexcept { + return std::uint64_t{1} << a.index(); + } + std::uint64_t mask() const noexcept { + return facts_.load(std::memory_order_acquire) | + (derived_.load(std::memory_order_acquire) & mask_bits); + } + template + static void collect(set_type &out, std::uint64_t m, + std::index_sequence) { + ((m & (std::uint64_t{1} << I) + ? (void)out.insert( + std::variant_alternative_t{}) + : void()), + ...); + } + static set_type set_from_mask(std::uint64_t m) { + set_type out; + collect( + out, m, + std::make_index_sequence>{}); + return out; + } + void assign_from(numeric_assumption_manager const &o) noexcept { + facts_.store(o.facts_.load(std::memory_order_acquire), + std::memory_order_release); + derived_.store(o.derived_.load(std::memory_order_acquire), + std::memory_order_release); + intrinsic_.store(o.intrinsic_.load(std::memory_order_acquire), + std::memory_order_release); + attached_ = o.attached_; + } void invalidate_dependents() noexcept { if (attached_) detail::assumption_epoch.fetch_add(1, std::memory_order_relaxed); } - std::set set_; - bool inferred_{false}; + std::atomic facts_{0}; + std::atomic derived_{0}; + std::atomic intrinsic_{false}; bool attached_{false}; - std::uint64_t epoch_{0}; }; // Manager for tensor algebra-property assumptions (orthogonal, PD, PSD). diff --git a/include/numsim_cas/core/expression.h b/include/numsim_cas/core/expression.h index c1512dff..a82dd890 100644 --- a/include/numsim_cas/core/expression.h +++ b/include/numsim_cas/core/expression.h @@ -2,6 +2,7 @@ #define EXPRESSION_H #include "assumptions.h" +#include #include namespace numsim::cas { @@ -44,7 +45,8 @@ class expression { * identity in the current model — it's user-asserted metadata). */ expression(expression const &data) - : m_assumption(data.m_assumption), m_hash_value(data.m_hash_value) { + : m_assumption(data.m_assumption), m_hash_value(data.m_hash_value), + m_hash_state(data.published_hash_state()) { m_assumption.attach_to_node(); } @@ -54,7 +56,8 @@ class expression { */ expression(expression &&data) noexcept : m_assumption(std::move(data.m_assumption)), - m_hash_value(data.m_hash_value) { + m_hash_value(data.m_hash_value), + m_hash_state(data.published_hash_state()) { m_assumption.attach_to_node(); } @@ -115,9 +118,25 @@ class expression { virtual void update_hash_value() const = 0; numeric_assumption_manager m_assumption{}; - // NOTE: lazy hash caching is not thread-safe. If multithreading is - // introduced, protect update_hash_value() with synchronization. + // Overrides write m_hash_value; hash_value() publishes it exactly once + // through m_hash_state, so concurrent readers never see a partial value. mutable hash_type m_hash_value{0}; + + // Drop a cached hash after mutating a node's children. + void reset_hash() const noexcept { + m_hash_value = 0; + m_hash_state.store(hash_unset, std::memory_order_release); + } + +private: + enum : unsigned char { hash_unset = 0, hash_computing = 1, hash_ready = 2 }; + mutable std::atomic m_hash_state{hash_unset}; + // a copy inherits a ready hash; one still being computed is recomputed + unsigned char published_hash_state() const noexcept { + return m_hash_state.load(std::memory_order_acquire) == hash_ready + ? hash_ready + : hash_unset; + } }; } // namespace numsim::cas diff --git a/include/numsim_cas/core/n_ary_tree.h b/include/numsim_cas/core/n_ary_tree.h index 6f3af02b..e8afba15 100644 --- a/include/numsim_cas/core/n_ary_tree.h +++ b/include/numsim_cas/core/n_ary_tree.h @@ -79,7 +79,7 @@ template class n_ary_tree : public Base { // Copies carry the source's cached hash; any mutation must drop it or // == fast-rejects on the stale value and cancellation silently fails. - inline void invalidate_hash() noexcept { this->m_hash_value = 0; } + inline void invalidate_hash() noexcept { this->reset_hash(); } // Insert `entry`, combining with any colliding map entry first. // After combination, `+` may algebraically simplify to an expression with a diff --git a/src/numsim_cas/core/expression.cpp b/src/numsim_cas/core/expression.cpp index ad79e4bf..e6dcad8b 100644 --- a/src/numsim_cas/core/expression.cpp +++ b/src/numsim_cas/core/expression.cpp @@ -1,14 +1,31 @@ #include +#include #include namespace numsim::cas { expression::hash_type const &expression::hash_value() const { - if (!m_hash_value) { - update_hash_value(); + for (;;) { + auto state = m_hash_state.load(std::memory_order_acquire); + if (state == hash_ready) + return m_hash_value; + if (state == hash_unset && + m_hash_state.compare_exchange_strong(state, hash_computing, + std::memory_order_acq_rel)) { + try { + update_hash_value(); + } catch (...) { + // let a waiter take over rather than spin on a value nobody computes + m_hash_state.store(hash_unset, std::memory_order_release); + throw; + } + m_hash_state.store(hash_ready, std::memory_order_release); + return m_hash_value; + } + if (state == hash_computing) + std::this_thread::yield(); } - return m_hash_value; } bool expression::operator==(expression const &rhs) const { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ef155f92..ee1d7796 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -58,6 +58,7 @@ add_numsim_cas_test(numsim_cas_test TensorToScalarMulOperatorTest.h TensorToScalarEvaluatorTest.h TensorToScalarExpressionTest.h + ThreadSafetyTest.h TensorToScalarSubstitutionTest.h ScalarLatexPrinterTest.h TensorLatexPrinterTest.h diff --git a/tests/ThreadSafetyTest.h b/tests/ThreadSafetyTest.h new file mode 100644 index 00000000..07c3db12 --- /dev/null +++ b/tests/ThreadSafetyTest.h @@ -0,0 +1,125 @@ +#ifndef THREADSAFETYTEST_H +#define THREADSAFETYTEST_H + +#include "cas_test_helpers.h" +#include "numsim_cas/numsim_cas.h" +#include "gtest/gtest.h" + +#include +#include +#include + +// Concurrent reads of one shared expression: the lazily cached hash and the +// inferred assumptions are written on first read, so these are only sound if +// that first write is published safely. +namespace numsim::cas { + +namespace { +template void run_threads(std::size_t count, Fn &&fn) { + std::vector workers; + workers.reserve(count); + for (std::size_t t = 0; t < count; ++t) + workers.emplace_back(fn, t); + for (auto &w : workers) + w.join(); +} +constexpr std::size_t threads = 4; +constexpr int rounds = 200; +} // namespace + +TEST(ThreadSafety, ConcurrentHashReadsOnSharedNode) { + auto [x] = make_scalar_variable("x"); + // built directly, so nothing has hashed it yet + auto lazy = make_expression(make_expression(x)); + auto const expected = + make_expression(make_expression(x)) + .get() + .hash_value(); + std::atomic mismatches{0}; + run_threads(threads, [&](std::size_t) { + for (int i = 0; i < rounds; ++i) + if (lazy.get().hash_value() != expected) + mismatches.fetch_add(1, std::memory_order_relaxed); + }); + EXPECT_EQ(mismatches.load(), 0); +} + +TEST(ThreadSafety, ConcurrentHashReadsOnSharedTensorNode) { + auto [A] = + make_tensor_variable(std::tuple{"A", std::size_t{3}, std::size_t{2}}); + auto lazy = make_expression(make_expression(A)); + auto const expected = + make_expression(make_expression(A)) + .get() + .hash_value(); + std::atomic mismatches{0}; + run_threads(threads, [&](std::size_t) { + for (int i = 0; i < rounds; ++i) + if (lazy.get().hash_value() != expected) + mismatches.fetch_add(1, std::memory_order_relaxed); + }); + EXPECT_EQ(mismatches.load(), 0); +} + +TEST(ThreadSafety, ConcurrentAssumptionQueriesOnSharedNode) { + auto [x, y] = make_scalar_variable("x", "y"); + x.assumption(positive{}); + auto const build = [&] { + return abs(x) + sqrt(y * y + make_expression(1)); + }; + // the reference is a separate tree, so the shared one reaches the threads + // with its facts still underived and they race to infer them + auto const reference = build(); + bool const pos = is_positive(reference); + bool const neg = is_negative(reference); + bool const nonneg = is_nonnegative(reference); + + auto shared = build(); + std::atomic wrong{0}; + run_threads(threads, [&](std::size_t) { + for (int i = 0; i < rounds; ++i) { + if (is_positive(shared) != pos || is_negative(shared) != neg || + is_nonnegative(shared) != nonneg) + wrong.fetch_add(1, std::memory_order_relaxed); + } + }); + EXPECT_EQ(wrong.load(), 0); + EXPECT_TRUE(is_positive(x)); +} + +TEST(ThreadSafety, ConcurrentTensorQueriesOnSharedNode) { + auto [F] = + make_tensor_variable(std::tuple{"F", std::size_t{3}, std::size_t{2}}); + auto C = trans(F) * F; + std::atomic wrong{0}; + run_threads(threads, [&](std::size_t) { + for (int i = 0; i < rounds; ++i) { + if (!is_symmetric(C) || is_skew(C)) + wrong.fetch_add(1, std::memory_order_relaxed); + } + }); + EXPECT_EQ(wrong.load(), 0); +} + +TEST(ThreadSafety, ConcurrentEvaluationWithPerThreadEvaluators) { + auto [x, y] = make_scalar_variable("x", "y"); + auto f = + sin(x) * exp(y) + pow(x, 3) / (y + make_expression(2)); + std::atomic wrong{0}; + run_threads(threads, [&](std::size_t t) { + scalar_evaluator ev; + double const xv = 0.5 + static_cast(t); + ev.set(x, xv); + ev.set(y, 1.5); + double const reference = + std::sin(xv) * std::exp(1.5) + std::pow(xv, 3) / 3.5; + for (int i = 0; i < rounds; ++i) + if (std::abs(ev.apply(f) - reference) > 1e-12) + wrong.fetch_add(1, std::memory_order_relaxed); + }); + EXPECT_EQ(wrong.load(), 0); +} + +} // namespace numsim::cas + +#endif // THREADSAFETYTEST_H diff --git a/tests/main.cpp b/tests/main.cpp index 8fa4a722..38d2792a 100644 --- a/tests/main.cpp +++ b/tests/main.cpp @@ -32,6 +32,7 @@ #include "TensorToScalarMulOperatorTest.h" #include "TensorToScalarPrinterTest.h" #include "TensorToScalarSubstitutionTest.h" +#include "ThreadSafetyTest.h" #include "gtest/gtest.h" int main(int argc, char *argv[]) { From 9ed96c20bba2c211b4ae9299ed74d7d88aa0717f Mon Sep 17 00:00:00 2001 From: petlenz Date: Sun, 20 Sep 2026 22:12:33 +0200 Subject: [PATCH 2/2] Carry the epoch semantics onto the atomic fact masks 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 --- include/numsim_cas/core/assumptions.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/include/numsim_cas/core/assumptions.h b/include/numsim_cas/core/assumptions.h index 772997d0..d0ab7e95 100644 --- a/include/numsim_cas/core/assumptions.h +++ b/include/numsim_cas/core/assumptions.h @@ -184,6 +184,16 @@ class numeric_assumption_manager { } set_type data() const { return set_from_mask(mask()); } + // The facts as currently believed, detached from any node: stale derived + // ones are dropped rather than re-derived. Domains without a propagator + // read through this. + numeric_assumption_manager effective() const { + numeric_assumption_manager m; + if (!stale(detail::current_assumption_epoch())) + m.facts_.store(mask(), std::memory_order_release); + return m; + } + // Facts the library establishes itself: intrinsic to a constant or // computed from children. They invalidate nothing. void insert_derived(numeric_assumption a) {