From 19af8da65c303b2ccdaf936f29182eb691081eaf Mon Sep 17 00:00:00 2001
From: Andrew Noyes <andrew@weaselab.dev>
Date: Mon, 5 Aug 2024 17:50:26 -0700
Subject: [PATCH] Fix endNode's max version after the fact

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 | 67 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/ConflictSet.cpp b/ConflictSet.cpp
index 110fec5..35df22f 100644
--- a/ConflictSet.cpp
+++ b/ConflictSet.cpp
@@ -2885,7 +2885,6 @@ checkMaxBetweenExclusiveImpl<true>(Node *n, int begin, int end,
 // `entryPresent`, `entry` fields and `maxVersion` on the result.  The search
 // path of the result's parent will have `maxVersion` at least `writeVersion` as
 // a postcondition. Nodes along the 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) {
@@ -2941,20 +2940,13 @@ template <bool kBegin>
       }
     }
 
-    if constexpr (kBegin) {
-      assert(maxVersion(*self, impl) <= writeVersion);
-      setMaxVersion(*self, impl, writeVersion);
-    }
+    assert(maxVersion(*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);
@@ -2963,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;
@@ -3000,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);
@@ -3018,6 +3010,45 @@ 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);
+    self48->bitSet.forEachSet([&](int i) {
+      max = std::max(self48->childMaxVersion[self48->index[i]], max);
+    });
+  } break;
+  case Type_Node256: {
+    auto *self256 = static_cast<Node256 *>(node);
+    self256->bitSet.forEachSet(
+        [&](int i) { max = std::max(self256->childMaxVersion[i], 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) {
@@ -3064,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;
 
@@ -3084,7 +3115,7 @@ void addWriteRange(Node *&root, std::span<const uint8_t> begin,
   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;
 
@@ -3096,22 +3127,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) {