Disable debug symbols and frame pointer for macos

This causes some versions of clang to crash

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.18)
 project(
   conflict-set
-  VERSION 0.0.10
+  VERSION 0.0.11
   DESCRIPTION
     "A data structure for optimistic concurrency control on ranges of bitwise-lexicographically-ordered keys."
   HOMEPAGE_URL "https://git.weaselab.dev/weaselab/conflict-set"
@@ -31,14 +31,12 @@ if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
                                                "MinSizeRel" "RelWithDebInfo")
 endif()
 
-add_compile_options(
-  -fdata-sections
-  -ffunction-sections
-  -Wswitch-enum
-  -Werror=switch-enum
-  -fPIC
-  -g
-  -fno-omit-frame-pointer)
+add_compile_options(-fdata-sections -ffunction-sections -Wswitch-enum
+                    -Werror=switch-enum -fPIC)
+if(NOT APPLE)
+  # This causes some versions of clang to crash on macos
+  add_compile_options(-g -fno-omit-frame-pointer)
+endif()
 
 set(full_relro_flags "-pie;LINKER:-z,relro,-z,now,-z,noexecstack")
 cmake_push_check_state()

ConflictSet.cpp

@@ -902,6 +902,7 @@ Node *&getChildExists(Node *self, uint8_t index) {
 }
 
 InternalVersionT maxVersion(Node *n, ConflictSet::Impl *);
+InternalVersionT exchangeMaxVersion(Node *n, InternalVersionT newMax);
 
 void setMaxVersion(Node *n, ConflictSet::Impl *, InternalVersionT maxVersion);
 
@@ -1123,8 +1124,10 @@ Node *getFirstChildExists(Node *self) {
 }
 
 // Caller is responsible for assigning a non-null pointer to the returned
-// reference if null
-Node *&getOrCreateChild(Node *&self, uint8_t index, WriteContext *tls) {
+// reference if null. Updates child's max version to `newMaxVersion` if child
+// exists but does not have a partial key.
+Node *&getOrCreateChild(Node *&self, uint8_t index,
+                        InternalVersionT newMaxVersion, WriteContext *tls) {
 
   // Fast path for if it exists already
   switch (self->getType()) {
@@ -1134,6 +1137,9 @@ Node *&getOrCreateChild(Node *&self, uint8_t index, WriteContext *tls) {
     auto *self3 = static_cast<Node3 *>(self);
     int i = getNodeIndex(self3, index);
     if (i >= 0) {
+      if (self3->children[i]->partialKeyLen == 0) {
+        self3->childMaxVersion[i] = newMaxVersion;
+      }
       return self3->children[i];
     }
   } break;
@@ -1141,6 +1147,9 @@ Node *&getOrCreateChild(Node *&self, uint8_t index, WriteContext *tls) {
     auto *self16 = static_cast<Node16 *>(self);
     int i = getNodeIndex(self16, index);
     if (i >= 0) {
+      if (self16->children[i]->partialKeyLen == 0) {
+        self16->childMaxVersion[i] = newMaxVersion;
+      }
       return self16->children[i];
     }
   } break;
@@ -1148,12 +1157,23 @@ Node *&getOrCreateChild(Node *&self, uint8_t index, WriteContext *tls) {
     auto *self48 = static_cast<Node48 *>(self);
     int secondIndex = self48->index[index];
     if (secondIndex >= 0) {
+      if (self48->children[secondIndex]->partialKeyLen == 0) {
+        self48->childMaxVersion[secondIndex] = newMaxVersion;
+        self48->maxOfMax[secondIndex >> Node48::kMaxOfMaxShift] =
+            std::max(self48->maxOfMax[secondIndex >> Node48::kMaxOfMaxShift],
+                     newMaxVersion);
+      }
       return self48->children[secondIndex];
     }
   } break;
   case Type_Node256: {
     auto *self256 = static_cast<Node256 *>(self);
     if (auto &result = self256->children[index]; result != nullptr) {
+      if (self256->children[index]->partialKeyLen == 0) {
+        self256->childMaxVersion[index] = newMaxVersion;
+        self256->maxOfMax[index >> Node256::kMaxOfMaxShift] = std::max(
+            self256->maxOfMax[index >> Node256::kMaxOfMaxShift], newMaxVersion);
+      }
       return result;
     }
   } break;
@@ -1828,43 +1848,6 @@ int longestCommonPrefix(const uint8_t *ap, const uint8_t *bp, int cl) {
   return i;
 }
 
-// Performs a physical search for remaining
-struct SearchStepWise {
-  Node *n;
-  InternalVersionT maxV;
-  std::span<const uint8_t> remaining;
-
-  SearchStepWise() {}
-  SearchStepWise(Node *n, std::span<const uint8_t> remaining)
-      : n(n), remaining(remaining) {
-    assert(n->partialKeyLen == 0);
-  }
-
-  bool step() {
-    if (remaining.size() == 0) {
-      return true;
-    }
-    auto [child, v] = getChildAndMaxVersion(n, remaining[0]);
-    maxV = v;
-    if (child == nullptr) {
-      return true;
-    }
-    if (child->partialKeyLen > 0) {
-      int cl = std::min<int>(child->partialKeyLen, remaining.size() - 1);
-      int i =
-          longestCommonPrefix(child->partialKey(), remaining.data() + 1, cl);
-      if (i != child->partialKeyLen) {
-        return true;
-      }
-    }
-    n = child;
-    remaining =
-        remaining.subspan(1 + child->partialKeyLen,
-                          remaining.size() - (1 + child->partialKeyLen));
-    return false;
-  }
-};
-
 // Logically this is the same as performing firstGeq and then checking against
 // point or range version according to cmp, but this version short circuits as
 // soon as it can prove that there's no conflict.
@@ -2121,13 +2104,9 @@ bool scan16(const InternalVersionT *vs, const uint8_t *is, int begin, int end,
 }
 
 // Returns true if v[i] <= readVersion for all i such that begin <= i < end
-//
-// always_inline So that we can optimize when begin or end is a constant.
-// gcovr exclude annotation necessary because of always_inline?
 template <bool kAVX512>
-inline __attribute__((always_inline)) bool
-scan16(const InternalVersionT *vs, int begin, int end, // GCOVR_EXCL_LINE
-       InternalVersionT readVersion) {                 // GCOVR_EXCL_LINE
+bool scan16(const InternalVersionT *vs, int begin, int end,
+            InternalVersionT readVersion) {
   assert(0 <= begin && begin < 16);
   assert(0 <= end && end <= 16);
   assert(begin <= end);
@@ -2799,30 +2778,47 @@ bool checkRangeRead(Node *n, std::span<const uint8_t> begin,
 
   ++tls->range_read_accum;
 
-  SearchStepWise search{n, begin.subspan(0, lcp)};
+  auto remaining = begin.subspan(0, lcp);
   Arena arena;
+  // If the common prefix isn't a prefix of any physical entry in the tree, we
+  // can go to "downLeftSpine"
   for (;; ++tls->range_read_iterations_accum) {
-    assert(getSearchPath(arena, search.n) <=>
-               begin.subspan(0, lcp - search.remaining.size()) ==
+    assert(getSearchPath(arena, n) <=>
+               begin.subspan(0, lcp - remaining.size()) ==
            0);
-    if (search.step()) {
+    if (remaining.size() == 0) {
       break;
     }
-    if (search.maxV <= readVersion) {
+    auto [child, v] = getChildAndMaxVersion(n, remaining[0]);
+    if (child == nullptr) {
+      break;
+    }
+
+    if (child->partialKeyLen > 0) {
+      int cl = std::min<int>(child->partialKeyLen, remaining.size() - 1);
+      int i =
+          longestCommonPrefix(child->partialKey(), remaining.data() + 1, cl);
+      if (i != child->partialKeyLen) {
+        break;
+      }
+    }
+    if (v <= readVersion) {
       ++tls->range_read_short_circuit_accum;
       return true;
     }
+    n = child;
+    remaining =
+        remaining.subspan(1 + child->partialKeyLen,
+                          remaining.size() - (1 + child->partialKeyLen));
   }
-  assert(getSearchPath(arena, search.n) <=>
-             begin.subspan(0, lcp - search.remaining.size()) ==
+  assert(getSearchPath(arena, n) <=> begin.subspan(0, lcp - remaining.size()) ==
          0);
 
-  const int consumed = lcp - search.remaining.size();
+  const int consumed = lcp - remaining.size();
   assume(consumed >= 0);
 
   begin = begin.subspan(consumed, int(begin.size()) - consumed);
   end = end.subspan(consumed, int(end.size()) - consumed);
-  n = search.n;
   lcp -= consumed;
 
   if (lcp == int(begin.size())) {
@@ -2873,7 +2869,7 @@ bool checkRangeRead(Node *n, std::span<const uint8_t> begin,
 template __attribute__((target("avx512f"))) bool
 scan16<true>(const InternalVersionT *vs, const uint8_t *is, int begin, int end,
              InternalVersionT readVersion);
-template __attribute__((always_inline, target("avx512f"))) bool
+template __attribute__((target("avx512f"))) bool
 scan16<true>(const InternalVersionT *vs, int begin, int end,
              InternalVersionT readVersion);
 template __attribute__((target("avx512f"))) bool
@@ -2881,85 +2877,93 @@ checkMaxBetweenExclusiveImpl<true>(Node *n, int begin, int end,
                                    InternalVersionT readVersion, ReadContext *);
 #endif
 
+// Consume the partial key of `self` (which must exist), and update `self` and
+// `key` such that `self` is along the search path of `key`
+void consumePartialKey(Node *&self, std::span<const uint8_t> &key,
+                       InternalVersionT writeVersion, WriteContext *tls) {
+  assert(self->partialKeyLen > 0);
+  // Handle an existing partial key
+  int commonLen = std::min<int>(self->partialKeyLen, key.size());
+  int partialKeyIndex =
+      longestCommonPrefix(self->partialKey(), key.data(), commonLen);
+  if (partialKeyIndex < self->partialKeyLen) {
+    auto *old = self;
+    // Since root cannot have a partial key
+    assert(old->parent != nullptr);
+    InternalVersionT oldMaxVersion = exchangeMaxVersion(old, writeVersion);
+
+    // *self will have one child (old)
+    auto *newSelf = tls->allocate<Node3>(partialKeyIndex);
+
+    newSelf->parent = old->parent;
+    newSelf->parentsIndex = old->parentsIndex;
+    newSelf->partialKeyLen = partialKeyIndex;
+    newSelf->entryPresent = false;
+    newSelf->numChildren = 1;
+
+    memcpy(newSelf->partialKey(), old->partialKey(), newSelf->partialKeyLen);
+
+    uint8_t oldDistinguishingByte = old->partialKey()[partialKeyIndex];
+    old->parent = newSelf;
+    old->parentsIndex = oldDistinguishingByte;
+    newSelf->index[0] = oldDistinguishingByte;
+    newSelf->children[0] = old;
+    newSelf->childMaxVersion[0] = oldMaxVersion;
+    self = newSelf;
+
+    memmove(old->partialKey(), old->partialKey() + partialKeyIndex + 1,
+            old->partialKeyLen - (partialKeyIndex + 1));
+    old->partialKeyLen -= partialKeyIndex + 1;
+
+    // We would consider decreasing capacity here, but we can't invalidate
+    // old since it's not on the search path. setOldestVersion will clean it
+    // up.
+  }
+  key = key.subspan(partialKeyIndex, key.size() - partialKeyIndex);
+}
+
 // Returns a pointer to the newly inserted node.  Caller must set
-// `entryPresent`, and `entry` fields.  The search path of the result will have
-// `maxVersion` at least `writeVersion` as a postcondition. Nodes along the
-// search path to `key` may be invalidated.
-[[nodiscard]] Node *insert(Node **self, std::span<const uint8_t> key,
-                           InternalVersionT writeVersion, WriteContext *tls,
-                           ConflictSet::Impl *impl) {
+// `entryPresent`, and `entry` fields.  All nodes along the search path of the
+// result will have `maxVersion` set to `writeVersion` as a postcondition. Nodes
+// along the search path may be invalidated.
+[[nodiscard]]
+Node *insert(Node **self, std::span<const uint8_t> key,
+             InternalVersionT writeVersion, WriteContext *tls,
+             ConflictSet::Impl *impl) {
+
+  if ((*self)->partialKeyLen > 0) {
+    consumePartialKey(*self, key, writeVersion, tls);
+  }
+  assert(maxVersion(*self, impl) <= writeVersion);
+  setMaxVersion(*self, impl, writeVersion);
 
   for (;; ++tls->accum.insert_iterations) {
 
-    if ((*self)->partialKeyLen > 0) {
-      // Handle an existing partial key
-      int commonLen = std::min<int>((*self)->partialKeyLen, key.size());
-      int partialKeyIndex =
-          longestCommonPrefix((*self)->partialKey(), key.data(), commonLen);
-      if (partialKeyIndex < (*self)->partialKeyLen) {
-        auto *old = *self;
-        InternalVersionT oldMaxVersion = maxVersion(old, impl);
-
-        // *self will have one child
-        *self = tls->allocate<Node3>(partialKeyIndex);
-
-        memcpy((char *)*self + kNodeCopyBegin, (char *)old + kNodeCopyBegin,
-               kNodeCopySize);
-        (*self)->partialKeyLen = partialKeyIndex;
-
-        // Not necessary to call removeKey here, since this node is "synthetic"
-        (*self)->entryPresent = false;
-
-        (*self)->numChildren = 0;
-        memcpy((*self)->partialKey(), old->partialKey(),
-               (*self)->partialKeyLen);
-
-        getOrCreateChild(*self, old->partialKey()[partialKeyIndex], tls) = old;
-        old->parent = *self;
-        old->parentsIndex = old->partialKey()[partialKeyIndex];
-        setMaxVersion(old, impl, oldMaxVersion);
-
-        memmove(old->partialKey(), old->partialKey() + partialKeyIndex + 1,
-                old->partialKeyLen - (partialKeyIndex + 1));
-        old->partialKeyLen -= partialKeyIndex + 1;
-
-        // We would consider decreasing capacity here, but we can't invalidate
-        // old since it's not on the search path. setOldestVersion will clean it
-        // up.
-      }
-      key = key.subspan(partialKeyIndex, key.size() - partialKeyIndex);
-
-    } else {
-      // Consider adding a partial key
-      if ((*self)->numChildren == 0 && !(*self)->entryPresent) {
-        assert((*self)->getCapacity() >= int(key.size()));
-        (*self)->partialKeyLen = key.size();
-        memcpy((*self)->partialKey(), key.data(), (*self)->partialKeyLen);
-        key = key.subspan((*self)->partialKeyLen,
-                          key.size() - (*self)->partialKeyLen);
-      }
-    }
-
-    assert(maxVersion(*self, impl) <= writeVersion);
-    setMaxVersion(*self, impl, writeVersion);
-
     if (key.size() == 0) {
       return *self;
     }
 
-    auto &child = getOrCreateChild(*self, key.front(), tls);
+    auto &child = getOrCreateChild(*self, key.front(), writeVersion, tls);
     if (!child) {
       child = tls->allocate<Node0>(key.size() - 1);
       child->numChildren = 0;
       child->entryPresent = false;
-      child->partialKeyLen = 0;
+      child->partialKeyLen = key.size() - 1;
       child->parent = *self;
       child->parentsIndex = key.front();
-      setMaxVersion(child, impl, tls->zero);
+      setMaxVersion(child, impl, writeVersion);
+      memcpy(child->partialKey(), key.data() + 1, child->partialKeyLen);
+      return child;
     }
 
     self = &child;
     key = key.subspan(1, key.size() - 1);
+
+    if ((*self)->partialKeyLen > 0) {
+      consumePartialKey(*self, key, writeVersion, tls);
+      assert(maxVersion(*self, impl) <= writeVersion);
+      setMaxVersion(*self, impl, writeVersion);
+    }
   }
 }
 
@@ -3009,13 +3013,11 @@ void addPointWrite(Node *&root, std::span<const uint8_t> key,
   }
 }
 
+// Precondition: `node->entryPresent`
 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;
-  }
+  assert(node->entryPresent);
+  max = std::max(node->entry.pointVersion, tls->zero);
   switch (node->getType()) {
   case Type_Node0:
     break;
@@ -3556,6 +3558,39 @@ InternalVersionT maxVersion(Node *n, ConflictSet::Impl *impl) {
   }
 }
 
+// Precondition `n` is not the root
+InternalVersionT exchangeMaxVersion(Node *n, InternalVersionT newMax) {
+  int index = n->parentsIndex;
+  n = n->parent;
+  assert(n != nullptr);
+  switch (n->getType()) {
+  case Type_Node0:           // GCOVR_EXCL_LINE
+    __builtin_unreachable(); // GCOVR_EXCL_LINE
+  case Type_Node3: {
+    auto *n3 = static_cast<Node3 *>(n);
+    int i = getNodeIndex(n3, index);
+    return std::exchange(n3->childMaxVersion[i], newMax);
+  }
+  case Type_Node16: {
+    auto *n16 = static_cast<Node16 *>(n);
+    int i = getNodeIndex(n16, index);
+    return std::exchange(n16->childMaxVersion[i], newMax);
+  }
+  case Type_Node48: {
+    auto *n48 = static_cast<Node48 *>(n);
+    assert(n48->bitSet.test(index));
+    return std::exchange(n48->childMaxVersion[n48->index[index]], newMax);
+  }
+  case Type_Node256: {
+    auto *n256 = static_cast<Node256 *>(n);
+    assert(n256->bitSet.test(index));
+    return std::exchange(n256->childMaxVersion[index], newMax);
+  }
+  default:                   // GCOVR_EXCL_LINE
+    __builtin_unreachable(); // GCOVR_EXCL_LINE
+  }
+}
+
 void setMaxVersion(Node *n, ConflictSet::Impl *impl, InternalVersionT newMax) {
   int index = n->parentsIndex;
   n = n->parent;
@@ -4066,6 +4101,10 @@ checkMaxVersion(Node *root, Node *node, InternalVersionT oldestVersion,
                                        ConflictSet::Impl *impl) {
   bool success = true;
 
+  if (node->partialKeyLen > 0) {
+    fprintf(stderr, "Root cannot have a partial key");
+    success = false;
+  }
   checkParentPointers(node, success);
   checkMaxVersion(node, node, oldestVersion, success, impl);
   checkEntriesExist(node, success);

README.md

@@ -24,15 +24,15 @@ Hardware for all benchmarks is an AMD Ryzen 9 7900 with (2x32GB) 5600MT/s CL28-3
 
 |               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
 |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
-|               17.03 |       58,732,967.93 |    0.6% |          276.28 |           87.96 |  3.141 |          52.15 |    0.4% |      0.01 | `point reads`
-|               19.52 |       51,239,158.04 |    0.3% |          367.16 |          101.50 |  3.617 |          61.92 |    0.3% |      0.01 | `prefix reads`
-|               47.74 |       20,947,676.63 |    0.5% |          998.16 |          247.43 |  4.034 |         161.64 |    0.2% |      0.01 | `range reads`
-|               23.14 |       43,207,824.89 |    0.4% |          408.18 |          121.64 |  3.356 |          70.20 |    0.3% |      0.01 | `point writes`
-|               38.02 |       26,302,115.66 |    0.1% |          709.72 |          199.70 |  3.554 |         134.26 |    0.3% |      0.01 | `prefix writes`
-|               44.28 |       22,583,559.17 |    0.9% |          825.19 |          233.10 |  3.540 |         141.48 |    0.2% |      0.01 | `range writes`
-|               85.50 |       11,695,990.63 |    0.5% |        1,488.16 |          455.68 |  3.266 |         289.22 |    0.1% |      0.01 | `monotonic increasing point writes`
-|          338,388.50 |            2,955.18 |    3.3% |    4,097,087.00 |    1,809,996.00 |  2.264 |     759,645.00 |    0.0% |      0.01 | `worst case for radix tree`
-|               84.84 |       11,787,313.59 |    1.4% |        1,716.02 |          440.50 |  3.896 |         271.00 |    0.0% |      0.01 | `create and destroy`
+|               12.42 |       80,500,398.66 |    0.8% |          180.38 |           61.57 |  2.930 |          41.51 |    0.4% |      0.01 | `point reads`
+|               15.17 |       65,917,580.99 |    0.2% |          279.47 |           74.95 |  3.729 |          55.54 |    0.3% |      0.01 | `prefix reads`
+|               38.16 |       26,202,393.91 |    0.1% |          803.07 |          189.13 |  4.246 |         141.68 |    0.2% |      0.01 | `range reads`
+|               20.20 |       49,504,615.44 |    0.4% |          363.00 |          100.35 |  3.617 |          49.81 |    0.3% |      0.01 | `point writes`
+|               41.99 |       23,816,559.99 |    0.3% |          799.27 |          209.63 |  3.813 |         154.32 |    0.1% |      0.01 | `prefix writes`
+|               46.28 |       21,607,605.88 |    1.5% |          953.79 |          231.47 |  4.121 |         168.34 |    0.0% |      0.01 | `range writes`
+|               80.99 |       12,347,449.98 |    0.9% |        1,501.97 |          406.50 |  3.695 |         281.89 |    0.1% |      0.01 | `monotonic increasing point writes`
+|          318,010.00 |            3,144.56 |    1.0% |    3,994,511.50 |    1,657,831.50 |  2.409 |     805,969.50 |    0.0% |      0.01 | `worst case for radix tree`
+|               75.85 |       13,183,612.56 |    0.5% |        1,590.01 |          385.64 |  4.123 |         258.00 |    0.0% |      0.01 | `create and destroy`
 
 # "Real data" test
 
@@ -47,7 +47,7 @@ Check: 4.47891 seconds, 364.05 MB/s, Add: 4.55599 seconds, 123.058 MB/s, Gc rati
 ## radix tree
 
 ```
-Check: 1.05813 seconds, 1540.97 MB/s, Add: 1.32071 seconds, 424.508 MB/s, Gc ratio: 42.2067%
+Check: 0.963721 seconds, 1691.93 MB/s, Add: 1.3288 seconds, 421.924 MB/s, Gc ratio: 42.8819%
 ```
 
 ## hash table

corpus/0f5c5f28c31e44ee0ec085fec09adf0981065771

@@ -1,5 +0,0 @@
-
-<EFBFBD><EFBFBD>
-
-
-2

corpus/12bdd00fd4038756cbcf8ecdad1b0cd862603cd8

@@ -1 +0,0 @@
-<EFBFBD>