Avoid branching on type twice in erase

Use a new node member "endOfRange" instead

ConflictSet.cpp

@@ -173,7 +173,7 @@ int BitSet::firstSetGeq(int i) const {
   return -1;
 }
 
-enum Type {
+enum Type : int8_t {
   Type_Node0,
   Type_Node3,
   Type_Node16,
@@ -191,6 +191,8 @@ struct Node {
   int32_t partialKeyLen;
   int16_t numChildren;
   bool entryPresent;
+  // Temp variable used to signal the end of the range during addWriteRange
+  bool endOfRange;
   uint8_t parentsIndex;
   /* end section that's copied to the next node */
 
@@ -625,6 +627,7 @@ template <class T> struct BoundedFreeListAllocator {
 
   T *allocate(int partialKeyCapacity) {
     T *result = allocate_helper(partialKeyCapacity);
+    result->endOfRange = false;
     if constexpr (!std::is_same_v<T, Node0>) {
       memset(result->children, 0, sizeof(result->children));
       const auto z = InternalVersionT::zero;
@@ -1672,53 +1675,67 @@ void mergeWithChild(Node *&self, WriteContext *tls, ConflictSet::Impl *impl,
   tls->release(self3);
 }
 
-void maybeDownsize(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
+bool needsDownsize(Node *n) {
-                   Node *&dontInvalidate) {
+  static int minTable[] = {0, kMinChildrenNode3, kMinChildrenNode16,
+                           kMinChildrenNode48, kMinChildrenNode256};
+  return n->numChildren + n->entryPresent < minTable[n->getType()];
+}
 
-#if DEBUG_VERBOSE && !defined(NDEBUG)
+void downsize(Node3 *self, WriteContext *tls, ConflictSet::Impl *impl,
-  fprintf(stderr, "maybeDownsize: %s\n", getSearchPathPrintable(self).c_str());
+              Node *&dontInvalidate) {
-#endif
+  if (self->numChildren == 0) {
+    auto *newSelf = tls->allocate<Node0>(self->partialKeyLen);
+    newSelf->copyChildrenAndKeyFrom(*self);
+    getInTree(self, impl) = newSelf;
+    tls->release(self);
+  } else {
+    assert(self->numChildren == 1 && !self->entryPresent);
+    mergeWithChild(getInTree(self, impl), tls, impl, dontInvalidate, self);
+  }
+}
+
+void downsize(Node16 *self, WriteContext *tls, ConflictSet::Impl *impl) {
+  assert(self->numChildren + int(self->entryPresent) < kMinChildrenNode16);
+  auto *newSelf = tls->allocate<Node3>(self->partialKeyLen);
+  newSelf->copyChildrenAndKeyFrom(*self);
+  getInTree(self, impl) = newSelf;
+  tls->release(self);
+}
+
+void downsize(Node48 *self, WriteContext *tls, ConflictSet::Impl *impl) {
+  assert(self->numChildren + int(self->entryPresent) < kMinChildrenNode48);
+  auto *newSelf = tls->allocate<Node16>(self->partialKeyLen);
+  newSelf->copyChildrenAndKeyFrom(*self);
+  getInTree(self, impl) = newSelf;
+  tls->release(self);
+}
+
+void downsize(Node256 *self, WriteContext *tls, ConflictSet::Impl *impl) {
+  assert(self->numChildren + int(self->entryPresent) < kMinChildrenNode256);
+  auto *self256 = (Node256 *)self;
+  auto *newSelf = tls->allocate<Node48>(self->partialKeyLen);
+  newSelf->copyChildrenAndKeyFrom(*self256);
+  getInTree(self, impl) = newSelf;
+  tls->release(self256);
+}
+
+void downsize(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
+              Node *&dontInvalidate) {
 
   switch (self->getType()) {
   case Type_Node0:           // GCOVR_EXCL_LINE
     __builtin_unreachable(); // GCOVR_EXCL_LINE
-  case Type_Node3: {
+  case Type_Node3:
-    auto *self3 = (Node3 *)self;
+    downsize(static_cast<Node3 *>(self), tls, impl, dontInvalidate);
-    if (self->numChildren == 0) {
+    break;
-      auto *newSelf = tls->allocate<Node0>(self->partialKeyLen);
-      newSelf->copyChildrenAndKeyFrom(*self3);
-      getInTree(self, impl) = newSelf;
-      tls->release(self3);
-    } else if (self->numChildren == 1 && !self->entryPresent) {
-      mergeWithChild(getInTree(self, impl), tls, impl, dontInvalidate, self3);
-    }
-  } break;
   case Type_Node16:
-    if (self->numChildren + int(self->entryPresent) < kMinChildrenNode16) {
+    downsize(static_cast<Node16 *>(self), tls, impl);
-      auto *self16 = (Node16 *)self;
-      auto *newSelf = tls->allocate<Node3>(self->partialKeyLen);
-      newSelf->copyChildrenAndKeyFrom(*self16);
-      getInTree(self, impl) = newSelf;
-      tls->release(self16);
-    }
     break;
   case Type_Node48:
-    if (self->numChildren + int(self->entryPresent) < kMinChildrenNode48) {
+    downsize(static_cast<Node48 *>(self), tls, impl);
-      auto *self48 = (Node48 *)self;
-      auto *newSelf = tls->allocate<Node16>(self->partialKeyLen);
-      newSelf->copyChildrenAndKeyFrom(*self48);
-      getInTree(self, impl) = newSelf;
-      tls->release(self48);
-    }
     break;
   case Type_Node256:
-    if (self->numChildren + int(self->entryPresent) < kMinChildrenNode256) {
+    downsize(static_cast<Node256 *>(self), tls, impl);
-      auto *self256 = (Node256 *)self;
-      auto *newSelf = tls->allocate<Node48>(self->partialKeyLen);
-      newSelf->copyChildrenAndKeyFrom(*self256);
-      getInTree(self, impl) = newSelf;
-      tls->release(self256);
-    }
     break;
   default:                   // GCOVR_EXCL_LINE
     __builtin_unreachable(); // GCOVR_EXCL_LINE
@@ -1727,11 +1744,10 @@ void maybeDownsize(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
 
 // Precondition: self is not the root. May invalidate nodes along the search
 // path to self. May invalidate children of self->parent. Returns a pointer to
-// the node after self. If erase invalidates the pointee of `dontInvalidate`, it
+// the node after self. Precondition: `self->entryPresent`
-// will update it to its new pointee as well. Precondition: `self->entryPresent`
+
-template <bool kCheckDontInvalidate = true>
 Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
-            bool logical, Node *&dontInvalidate) {
+            bool logical) {
   ++tls->accum.entries_erased;
   assert(self->parent != nullptr);
 
@@ -1749,12 +1765,8 @@ Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
   self->entryPresent = false;
 
   if (self->numChildren != 0) {
-    const bool update = result == dontInvalidate;
+    if (needsDownsize(self)) {
-    maybeDownsize(self, tls, impl, result);
+      downsize(self, tls, impl, result);
-    if constexpr (kCheckDontInvalidate) {
-      if (update) {
-        dontInvalidate = result;
-      }
     }
     return result;
   }
@@ -1775,7 +1787,10 @@ Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
       parent3->children[i] = parent3->children[i + 1];
       parent3->childMaxVersion[i] = parent3->childMaxVersion[i + 1];
     }
-    assert(parent->numChildren > 0 || parent->entryPresent);
+
+    if (needsDownsize(parent3)) {
+      downsize(parent3, tls, impl, result);
+    }
   } break;
   case Type_Node16: {
     auto *parent16 = static_cast<Node16 *>(parent);
@@ -1788,8 +1803,9 @@ Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
       parent16->childMaxVersion[i] = parent16->childMaxVersion[i + 1];
     }
 
-    // By kMinChildrenNode16
+    if (needsDownsize(parent16)) {
-    assert(parent->numChildren > 0);
+      downsize(parent16, tls, impl, result);
+    }
   } break;
   case Type_Node48: {
     auto *parent48 = static_cast<Node48 *>(parent);
@@ -1817,8 +1833,9 @@ Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
 
     --parent->numChildren;
 
-    // By kMinChildrenNode48
+    if (needsDownsize(parent48)) {
-    assert(parent->numChildren > 0);
+      downsize(parent48, tls, impl, result);
+    }
   } break;
   case Type_Node256: {
     auto *parent256 = static_cast<Node256 *>(parent);
@@ -1827,31 +1844,17 @@ Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
 
     --parent->numChildren;
 
-    // By kMinChildrenNode256
+    if (needsDownsize(parent256)) {
-    assert(parent->numChildren > 0);
+      downsize(parent256, tls, impl, result);
-
+    }
   } break;
   default:                   // GCOVR_EXCL_LINE
     __builtin_unreachable(); // GCOVR_EXCL_LINE
   }
 
-  const bool update = result == dontInvalidate;
-  maybeDownsize(parent, tls, impl, result);
-  if constexpr (kCheckDontInvalidate) {
-    if (update) {
-      dontInvalidate = result;
-    }
-  }
-
   return result;
 }
 
-Node *erase(Node *self, WriteContext *tls, ConflictSet::Impl *impl,
-            bool logical) {
-  Node *dummy;
-  return erase<false>(self, tls, impl, logical, dummy);
-}
-
 Node *nextSibling(Node *node) {
   for (;;) {
     if (node->parent == nullptr) {
@@ -3064,13 +3067,21 @@ void addWriteRange(Node *&root, std::span<const uint8_t> begin,
   }
   endNode->entry.rangeVersion = writeVersion;
 
-  for (beginNode = nextLogical(beginNode); beginNode != endNode;
+  // Erase nodes in range
-       beginNode = erase(beginNode, tls, impl, /*logical*/ true, endNode)) {
+  assert(!beginNode->endOfRange);
+  assert(!endNode->endOfRange);
+  endNode->endOfRange = true;
+  auto *iter = beginNode;
+  for (iter = nextLogical(iter); !iter->endOfRange;
+       iter = erase(iter, tls, impl, /*logical*/ true)) {
+    assert(!iter->endOfRange);
   }
+  assert(iter->endOfRange);
+  iter->endOfRange = false;
 
-  // Inserting end trashed endNode's maxVersion. Fix that. Safe to call since
+  // Inserting end trashed the last node's maxVersion. Fix that. Safe to call
-  // the end key always has non-zero size.
+  // since the end key always has non-zero size.
-  fixupMaxVersion(endNode, tls);
+  fixupMaxVersion(iter, tls);
 }
 
 Node *firstGeqPhysical(Node *n, const std::span<const uint8_t> key) {