Use maxOfMax in fixupMaxVersion

This sets us up to unconditionally update the max version along the
search path for inserts, and avoid dispatching on type twice per
iteration.

ConflictSet.cpp

@@ -939,6 +939,54 @@ Node *getChild(Node *self, uint8_t index) {
   }
 }
 
+struct ChildAndMaxVersion {
+  Node *child;
+  InternalVersionT maxVersion;
+};
+
+ChildAndMaxVersion getChildAndMaxVersion(Node0 *, uint8_t) { return {}; }
+ChildAndMaxVersion getChildAndMaxVersion(Node3 *self, uint8_t index) {
+  int i = getNodeIndex(self, index);
+  if (i < 0) {
+    return {};
+  }
+  return {self->children[i], self->childMaxVersion[i]};
+}
+ChildAndMaxVersion getChildAndMaxVersion(Node16 *self, uint8_t index) {
+  int i = getNodeIndex(self, index);
+  if (i < 0) {
+    return {};
+  }
+  return {self->children[i], self->childMaxVersion[i]};
+}
+ChildAndMaxVersion getChildAndMaxVersion(Node48 *self, uint8_t index) {
+  int i = self->index[index];
+  if (i < 0) {
+    return {};
+  }
+  return {self->children[i], self->childMaxVersion[i]};
+}
+ChildAndMaxVersion getChildAndMaxVersion(Node256 *self, uint8_t index) {
+  return {self->children[index], self->childMaxVersion[index]};
+}
+
+ChildAndMaxVersion getChildAndMaxVersion(Node *self, uint8_t index) {
+  switch (self->getType()) {
+  case Type_Node0:
+    return getChildAndMaxVersion(static_cast<Node0 *>(self), index);
+  case Type_Node3:
+    return getChildAndMaxVersion(static_cast<Node3 *>(self), index);
+  case Type_Node16:
+    return getChildAndMaxVersion(static_cast<Node16 *>(self), index);
+  case Type_Node48:
+    return getChildAndMaxVersion(static_cast<Node48 *>(self), index);
+  case Type_Node256:
+    return getChildAndMaxVersion(static_cast<Node256 *>(self), index);
+  default:                   // GCOVR_EXCL_LINE
+    __builtin_unreachable(); // GCOVR_EXCL_LINE
+  }
+}
+
 template <class NodeT> Node *getChildGeqSimd(NodeT *self, int child) {
   static_assert(std::is_same_v<NodeT, Node3> || std::is_same_v<NodeT, Node16>);
 
@@ -1783,6 +1831,7 @@ int longestCommonPrefix(const uint8_t *ap, const uint8_t *bp, int cl) {
 // Performs a physical search for remaining
 struct SearchStepWise {
   Node *n;
+  InternalVersionT maxV;
   std::span<const uint8_t> remaining;
 
   SearchStepWise() {}
@@ -1795,7 +1844,8 @@ struct SearchStepWise {
     if (remaining.size() == 0) {
       return true;
     }
-    auto *child = getChild(n, remaining[0]);
+    auto [child, v] = getChildAndMaxVersion(n, remaining[0]);
+    maxV = v;
     if (child == nullptr) {
       return true;
     }
@@ -1825,12 +1875,7 @@ bool checkPointRead(Node *n, const std::span<const uint8_t> key,
   fprintf(stderr, "Check point read: %s\n", printable(key).c_str());
 #endif
   auto remaining = key;
-  auto *impl = tls->impl;
   for (;; ++tls->point_read_iterations_accum) {
-    if (maxVersion(n, impl) <= readVersion) {
-      ++tls->point_read_short_circuit_accum;
-      return true;
-    }
     if (remaining.size() == 0) {
       if (n->entryPresent) {
         return n->entry.pointVersion <= readVersion;
@@ -1839,7 +1884,7 @@ bool checkPointRead(Node *n, const std::span<const uint8_t> key,
       goto downLeftSpine;
     }
 
-    auto *child = getChild(n, remaining[0]);
+    auto [child, maxV] = getChildAndMaxVersion(n, remaining[0]);
     if (child == nullptr) {
       auto c = getChildGeq(n, remaining[0]);
       if (c != nullptr) {
@@ -1881,6 +1926,11 @@ bool checkPointRead(Node *n, const std::span<const uint8_t> key,
         goto downLeftSpine;
       }
     }
+
+    if (maxV <= readVersion) {
+      ++tls->point_read_short_circuit_accum;
+      return true;
+    }
   }
 downLeftSpine:
   for (; !n->entryPresent; n = getFirstChildExists(n)) {
@@ -1900,17 +1950,11 @@ bool checkPrefixRead(Node *n, const std::span<const uint8_t> key,
   auto remaining = key;
   auto *impl = tls->impl;
   for (;; ++tls->prefix_read_iterations_accum) {
-    auto m = maxVersion(n, impl);
     if (remaining.size() == 0) {
-      return m <= readVersion;
+      return maxVersion(n, impl) <= readVersion;
     }
 
-    if (m <= readVersion) {
+    auto [child, maxV] = getChildAndMaxVersion(n, remaining[0]);
-      ++tls->prefix_read_short_circuit_accum;
-      return true;
-    }
-
-    auto *child = getChild(n, remaining[0]);
     if (child == nullptr) {
       auto c = getChildGeq(n, remaining[0]);
       if (c != nullptr) {
@@ -1956,6 +2000,11 @@ bool checkPrefixRead(Node *n, const std::span<const uint8_t> key,
         goto downLeftSpine;
       }
     }
+
+    if (maxV <= readVersion) {
+      ++tls->prefix_read_short_circuit_accum;
+      return true;
+    }
   }
 downLeftSpine:
   for (; !n->entryPresent; n = getFirstChildExists(n)) {
@@ -2511,10 +2560,6 @@ struct CheckRangeLeftSide {
   bool ok;
 
   bool step() {
-    if (maxVersion(n, impl) <= readVersion) {
-      ok = true;
-      return true;
-    }
     if (remaining.size() == 0) {
       assert(searchPathLen >= prefixLen);
       ok = maxVersion(n, impl) <= readVersion;
@@ -2528,7 +2573,7 @@ struct CheckRangeLeftSide {
       }
     }
 
-    auto *child = getChild(n, remaining[0]);
+    auto [child, maxV] = getChildAndMaxVersion(n, remaining[0]);
     if (child == nullptr) {
       auto c = getChildGeq(n, remaining[0]);
       if (c != nullptr) {
@@ -2591,6 +2636,10 @@ struct CheckRangeLeftSide {
         return true;
       }
     }
+    if (maxV <= readVersion) {
+      ok = true;
+      return true;
+    }
     return false;
   }
 
@@ -2752,18 +2801,17 @@ bool checkRangeRead(Node *n, std::span<const uint8_t> begin,
 
   SearchStepWise search{n, begin.subspan(0, lcp)};
   Arena arena;
-  auto *impl = tls->impl;
   for (;; ++tls->range_read_iterations_accum) {
     assert(getSearchPath(arena, search.n) <=>
                begin.subspan(0, lcp - search.remaining.size()) ==
            0);
-    if (maxVersion(search.n, impl) <= readVersion) {
-      ++tls->range_read_short_circuit_accum;
-      return true;
-    }
     if (search.step()) {
       break;
     }
+    if (search.maxV <= readVersion) {
+      ++tls->range_read_short_circuit_accum;
+      return true;
+    }
   }
   assert(getSearchPath(arena, search.n) <=>
              begin.subspan(0, lcp - search.remaining.size()) ==
@@ -2834,10 +2882,9 @@ checkMaxBetweenExclusiveImpl<true>(Node *n, int begin, int end,
 #endif
 
 // Returns a pointer to the newly inserted node.  Caller must set
-// `entryPresent`, `entry` fields and `maxVersion` on the result.  The search
+// `entryPresent`, and `entry` fields.  The search path of the result will have
-// path of the result's parent will have `maxVersion` at least `writeVersion` as
+// `maxVersion` at least `writeVersion` as a postcondition. Nodes along the
-// a postcondition. Nodes along the search path to `key` may be invalidated.
+// search path to `key` may be invalidated.
-template <bool kBegin>
 [[nodiscard]] Node *insert(Node **self, std::span<const uint8_t> key,
                            InternalVersionT writeVersion, WriteContext *tls,
                            ConflictSet::Impl *impl) {
@@ -2893,20 +2940,13 @@ template <bool kBegin>
       }
     }
 
-    if constexpr (kBegin) {
+    assert(maxVersion(*self, impl) <= writeVersion);
-      assert(maxVersion(*self, impl) <= writeVersion);
+    setMaxVersion(*self, impl, writeVersion);
-      setMaxVersion(*self, impl, writeVersion);
-    }
 
     if (key.size() == 0) {
       return *self;
     }
 
-    if constexpr (!kBegin) {
-      assert(maxVersion(*self, impl) <= writeVersion);
-      setMaxVersion(*self, impl, writeVersion);
-    }
-
     auto &child = getOrCreateChild(*self, key.front(), tls);
     if (!child) {
       child = tls->allocate<Node0>(key.size() - 1);
@@ -2915,7 +2955,7 @@ template <bool kBegin>
       child->partialKeyLen = 0;
       child->parent = *self;
       child->parentsIndex = key.front();
-      setMaxVersion(child, impl, kBegin ? writeVersion : tls->zero);
+      setMaxVersion(child, impl, tls->zero);
     }
 
     self = &child;
@@ -2952,7 +2992,7 @@ void addPointWrite(Node *&root, std::span<const uint8_t> key,
                    InternalVersionT writeVersion, WriteContext *tls,
                    ConflictSet::Impl *impl) {
   ++tls->accum.point_writes;
-  auto *n = insert<true>(&root, key, writeVersion, tls, impl);
+  auto *n = insert(&root, key, writeVersion, tls, impl);
   if (!n->entryPresent) {
     ++tls->accum.entries_inserted;
     auto *p = nextLogical(n);
@@ -2961,7 +3001,6 @@ void addPointWrite(Node *&root, std::span<const uint8_t> key,
     n->entryPresent = true;
 
     n->entry.pointVersion = writeVersion;
-    setMaxVersion(n, impl, writeVersion);
     n->entry.rangeVersion =
         p == nullptr ? tls->zero : std::max(p->entry.rangeVersion, tls->zero);
   } else {
@@ -2970,6 +3009,46 @@ void addPointWrite(Node *&root, std::span<const uint8_t> key,
   }
 }
 
+void fixupMaxVersion(Node *node, ConflictSet::Impl *impl, WriteContext *tls) {
+  InternalVersionT max;
+  if (node->entryPresent) {
+    max = std::max(node->entry.pointVersion, tls->zero);
+  } else {
+    max = tls->zero;
+  }
+  switch (node->getType()) {
+  case Type_Node0:
+    break;
+  case Type_Node3: {
+    auto *self3 = static_cast<Node3 *>(node);
+    for (int i = 0; i < self3->numChildren; ++i) {
+      max = std::max(self3->childMaxVersion[i], max);
+    }
+  } break;
+  case Type_Node16: {
+    auto *self16 = static_cast<Node16 *>(node);
+    for (int i = 0; i < self16->numChildren; ++i) {
+      max = std::max(self16->childMaxVersion[i], max);
+    }
+  } break;
+  case Type_Node48: {
+    auto *self48 = static_cast<Node48 *>(node);
+    for (auto v : self48->maxOfMax) {
+      max = std::max(v, max);
+    }
+  } break;
+  case Type_Node256: {
+    auto *self256 = static_cast<Node256 *>(node);
+    for (auto v : self256->maxOfMax) {
+      max = std::max(v, max);
+    }
+  } break;
+  default:                   // GCOVR_EXCL_LINE
+    __builtin_unreachable(); // GCOVR_EXCL_LINE
+  }
+  setMaxVersion(node, impl, max);
+}
+
 void addWriteRange(Node *&root, std::span<const uint8_t> begin,
                    std::span<const uint8_t> end, InternalVersionT writeVersion,
                    WriteContext *tls, ConflictSet::Impl *impl) {
@@ -3016,7 +3095,7 @@ void addWriteRange(Node *&root, std::span<const uint8_t> begin,
   begin = begin.subspan(consumed, begin.size() - consumed);
   end = end.subspan(consumed, end.size() - consumed);
 
-  auto *beginNode = insert<true>(useAsRoot, begin, writeVersion, tls, impl);
+  auto *beginNode = insert(useAsRoot, begin, writeVersion, tls, impl);
 
   const bool insertedBegin = !beginNode->entryPresent;
 
@@ -3029,14 +3108,11 @@ void addWriteRange(Node *&root, std::span<const uint8_t> begin,
     beginNode->entry.rangeVersion =
         p == nullptr ? tls->zero : std::max(p->entry.rangeVersion, tls->zero);
     beginNode->entry.pointVersion = writeVersion;
-    assert(maxVersion(beginNode, impl) <= writeVersion);
-    setMaxVersion(beginNode, impl, writeVersion);
   }
-  setMaxVersion(beginNode, impl, writeVersion);
   assert(writeVersion >= beginNode->entry.pointVersion);
   beginNode->entry.pointVersion = writeVersion;
 
-  auto *endNode = insert<false>(useAsRoot, end, writeVersion, tls, impl);
+  auto *endNode = insert(useAsRoot, end, writeVersion, tls, impl);
 
   const bool insertedEnd = !endNode->entryPresent;
 
@@ -3048,22 +3124,22 @@ void addWriteRange(Node *&root, std::span<const uint8_t> begin,
     auto *p = nextLogical(endNode);
     endNode->entry.pointVersion =
         p == nullptr ? tls->zero : std::max(p->entry.rangeVersion, tls->zero);
-    auto m = maxVersion(endNode, impl);
-    setMaxVersion(endNode, impl,
-                  std::max<InternalVersionT>(m, endNode->entry.pointVersion));
   }
   endNode->entry.rangeVersion = writeVersion;
 
   if (beginIsPrefix && insertedEnd) {
     // beginNode may have been invalidated when inserting end. TODO can we do
     // better?
-    beginNode = insert<true>(useAsRoot, begin, writeVersion, tls, impl);
+    beginNode = insert(useAsRoot, begin, writeVersion, tls, impl);
     assert(beginNode->entryPresent);
   }
 
   for (beginNode = nextLogical(beginNode); beginNode != endNode;
        beginNode = erase(beginNode, tls, impl, /*logical*/ true, endNode)) {
   }
+
+  // Inserting end trashed endNode's maxVersion. Fix that
+  fixupMaxVersion(endNode, impl, tls);
 }
 
 Node *firstGeqPhysical(Node *n, const std::span<const uint8_t> key) {