Fix #504: lazy hash and inferred assumptions are race-free on shared nodes

include/numsim_cas/core/assumptions.h

@@ -7,6 +7,7 @@
 #include <numsim_cas/numsim_cas_type_traits.h>
 #include <numsim_cas/tensor/sequence.h>
 #include <set>
+#include <utility>
 #include <variant>
 #include <vector>
 
@@ -129,99 +130,158 @@ 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, numeric_assumption_less>;
+
   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();
+    return (mask() & bit_of(a)) != 0;
   }
-  auto const &data() const { return set_; }
+  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.set_ = set_;
+      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) { 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
   // managers built while inferring must not.
   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<numeric_assumption> <= 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 <std::size_t... I>
+  static void collect(set_type &out, std::uint64_t m,
+                      std::index_sequence<I...>) {
+    ((m & (std::uint64_t{1} << I)
+          ? (void)out.insert(
+                std::variant_alternative_t<I, numeric_assumption>{})
+          : void()),
+     ...);
+  }
+  static set_type set_from_mask(std::uint64_t m) {
+    set_type out;
+    collect(
+        out, m,
+        std::make_index_sequence<std::variant_size_v<numeric_assumption>>{});
+    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<numeric_assumption, numeric_assumption_less> set_;
-  bool inferred_{false};
+  std::atomic<std::uint64_t> facts_{0};
+  std::atomic<std::uint64_t> derived_{0};
+  std::atomic<bool> intrinsic_{false};
   bool attached_{false};
-  std::uint64_t epoch_{0};
 };
 
 // Manager for tensor algebra-property assumptions (orthogonal, PD, PSD).

include/numsim_cas/core/expression.h

@@ -2,6 +2,7 @@
 #define EXPRESSION_H
 
 #include "assumptions.h"
+#include <atomic>
 #include <cstdlib>
 
 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<unsigned char> 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

include/numsim_cas/core/n_ary_tree.h

@@ -79,7 +79,7 @@ template <typename Base> 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

src/numsim_cas/core/expression.cpp

@@ -1,14 +1,31 @@
 #include <numsim_cas/core/expression.h>
 
+#include <thread>
 #include <typeinfo>
 
 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 {

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