Use rangeVersion indicating < instead of >

This should allow us to use firstGeq (which should be possible to make
more efficient), and generally make things nicer

ConflictSet.cpp

@@ -515,27 +515,6 @@ Node *&getOrCreateChild(Node *&self, uint8_t index) {
   }
 }
 
-void fixMaxVersion(Node *self) {
-  for (;;) {
-    int64_t result = std::numeric_limits<int64_t>::lowest();
-    if (self->entryPresent) {
-      result = std::max(result, self->entry.pointVersion);
-      result = std::max(result, self->entry.rangeVersion);
-    }
-    for (int i = getChildGeq(self, 0); i >= 0; i = getChildGeq(self, i + 1)) {
-      result = std::max(result, getChildExists(self, i)->maxVersion);
-    }
-    if (self->maxVersion == result) {
-      break;
-    }
-    self->maxVersion = result;
-    if (self->parent == nullptr) {
-      break;
-    }
-    self = self->parent;
-  }
-}
-
 // Precondition - an entry for index must exist in the node
 void eraseChild(Node *self, uint8_t index) {
   free(getChildExists(self, index));
@@ -761,6 +740,14 @@ Iterator lastLeq(Node *n, const std::span<const uint8_t> key) {
   return {l.n, l.cmp};
 }
 
+Iterator firstGeq(Node *n, const std::span<const uint8_t> key) {
+  auto result = lastLeq(n, key);
+  if (result.cmp == 0) {
+    return result;
+  }
+  return {nextLogical(result.n), 1};
+}
+
 // Returns a pointer to the newly inserted node.  caller is reponsible for
 // setting 'entry' fields on the result, which may have !entryPresent. The
 // search path for `key` will have maxVersion at least `writeVersion` as a
@@ -836,92 +823,61 @@ void destroyTree(Node *root) {
   }
 }
 
-struct CheckStepWise {
-  ConflictSet::Result *result;
-  const ConflictSet::ReadRange *read;
-  StepwiseLastLeq left;
-  StepwiseLastLeq right;
-
-  CheckStepWise() {}
-
-  CheckStepWise(Node *root, ConflictSet::Result *result,
-                const ConflictSet::ReadRange *r)
-      : result(result), read(r),
-        left(root, std::span<const uint8_t>(r->begin.p, r->begin.len)),
-        right(root, std::span<const uint8_t>(r->end.p, r->end.len)),
-        phase(r->end.len == 0 ? PointRead : RangeRead) {}
-
-  enum Phase {
-    PointRead,
-    RangeRead,
-  };
-
-  Phase phase;
-
-  bool step() {
-    switch (phase) {
-    case PointRead:
-      if (left.step()) {
-        auto *l = left.n;
-        int c = left.cmp;
-        assert(l != nullptr);
-        assert(l->entryPresent);
-        *result = (c == 0 ? l->entry.pointVersion : l->entry.rangeVersion) >
-                          read->readVersion
-                      ? ConflictSet::Conflict
-                      : ConflictSet::Commit;
-        return true;
-      }
-      return false;
-    case RangeRead: {
-      while (!left.step())
-        ;
-      while (!right.step())
-        ;
-      auto *l = left.n;
-      auto c = left.cmp;
-      *result = (c == 0 ? l->entry.pointVersion : l->entry.rangeVersion) >
-                        read->readVersion
-                    ? ConflictSet::Conflict
-                    : ConflictSet::Commit;
-      if (*result == ConflictSet::Commit) {
-        auto *e = right.n;
-        auto c = right.cmp;
-        if (l == e) {
-          return true;
-        }
-        if (c != 0) {
-          e = nextLogical(e);
-        }
-        for (auto iter = nextLogical(l); iter != e; iter = nextLogical(iter)) {
-          if (iter->entry.pointVersion > read->readVersion ||
-              iter->entry.rangeVersion > read->readVersion) {
-            *result = ConflictSet::Conflict;
-            return true;
-          }
-        }
-      }
-      return true;
-    }
-    }
-    __builtin_unreachable(); // GCOVR_EXCL_LINE
-  }
-};
-
 struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
 
   void check(const ReadRange *reads, Result *result, int count) const {
-    Arena arena;
-    CheckStepWise *checks = new (arena) CheckStepWise[count];
-    int index = 0;
     for (int i = 0; i < count; ++i) {
       if (reads[i].readVersion < oldestVersion) {
         result[i] = TooOld;
+        continue;
+      }
+      if (reads[i].end.len > 0) {
+        result[i] = Commit;
+        auto left = firstGeq(root, std::span<const uint8_t>(
+                                       reads[i].begin.p, reads[i].begin.len));
+        auto right = firstGeq(
+            root, std::span<const uint8_t>(reads[i].end.p, reads[i].end.len));
+        if (left.n != nullptr && left.cmp != 0 &&
+            left.n->entry.rangeVersion > reads[i].readVersion) {
+          result[i] = Conflict;
+          continue;
+        }
+        if (left.n == right.n) {
+          continue;
+        }
+        assert(left.n != nullptr);
+        auto boundaryVersion = left.n->entry.pointVersion;
+        if (left.cmp != 0) {
+          boundaryVersion =
+              std::max(boundaryVersion, left.n->entry.rangeVersion);
+        }
+        if (right.cmp == 0) {
+          boundaryVersion =
+              std::max(boundaryVersion, right.n->entry.rangeVersion);
+        }
+        if (boundaryVersion > reads[i].readVersion) {
+          result[i] = Conflict;
+          continue;
+        }
+        for (auto *iter = nextLogical(left.n); iter != right.n;
+             iter = nextLogical(iter)) {
+          if (std::max(iter->entry.pointVersion, iter->entry.rangeVersion) >
+              reads[i].readVersion) {
+            result[i] = Conflict;
+            break;
+          }
+        }
       } else {
-        checks[index++] = CheckStepWise(root, result + i, reads + i);
+        auto iter = firstGeq(root, std::span<const uint8_t>(
+                                       reads[i].begin.p, reads[i].begin.len));
+        result[i] = (iter.n == nullptr ? oldestVersion
+                     : iter.cmp == 0
+                         ? iter.n->entry.pointVersion
+                         : iter.n->entry.rangeVersion) > reads[i].readVersion
+                        ? Conflict
+                        : Commit;
       }
     }
-    runInterleaved(std::span<CheckStepWise>(checks, index));
   }
 
   void addWrites(const WriteRange *writes, int count) {
@@ -933,58 +889,52 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
                    w.writeVersion);
 
         const bool insertedBegin = !std::exchange(begin->entryPresent, true);
+
+        if (insertedBegin) {
+          auto *p = nextLogical(begin);
+          begin->entry.rangeVersion =
+              p != nullptr ? p->entry.rangeVersion : oldestVersion;
+        }
         begin->entry.pointVersion = w.writeVersion;
 
         auto *end = insert(&root, std::span<const uint8_t>(w.end.p, w.end.len),
-                           std::numeric_limits<int64_t>::lowest());
+                           w.writeVersion);
 
         const bool insertedEnd = !std::exchange(end->entryPresent, true);
+
+        if (insertedEnd) {
+          auto *p = nextLogical(end);
+          end->entry.pointVersion =
+              p != nullptr ? p->entry.rangeVersion : oldestVersion;
+        }
+        end->entry.rangeVersion = w.writeVersion;
+
         if (insertedEnd) {
           // begin may have been invalidated
           auto iter =
-              lastLeq(root, std::span<const uint8_t>(w.begin.p, w.begin.len));
+              firstGeq(root, std::span<const uint8_t>(w.begin.p, w.begin.len));
           assert(iter.cmp == 0);
           begin = iter.n;
         }
 
-        if (insertedBegin) {
-          auto *p = prevLogical(begin);
-          assert(p != nullptr);
-          begin->entry.rangeVersion = p->entry.rangeVersion;
-        }
-
-        if (insertedEnd) {
-          auto *p = prevLogical(end);
-          assert(p != nullptr);
-          end->entry.pointVersion = p->entry.rangeVersion;
-          end->entry.rangeVersion = p->entry.rangeVersion;
-        }
-
-        begin->entry.rangeVersion = w.writeVersion;
-
         for (begin = nextLogical(begin); begin != end;) {
           auto *old = begin;
           begin = nextLogical(begin);
           old->entryPresent = false;
-          fixMaxVersion(old);
           if (old->numChildren == 0 && old->parent != nullptr) {
             eraseChild(old->parent, old->parentsIndex);
           }
         }
-
-        if (insertedEnd) {
-          fixMaxVersion(end);
-        }
       } else {
         auto *n =
             insert(&root, std::span<const uint8_t>(w.begin.p, w.begin.len),
                    w.writeVersion);
         if (!n->entryPresent) {
-          auto *p = prevLogical(n);
+          auto *p = nextLogical(n);
-          assert(p != nullptr);
           n->entryPresent = true;
           n->entry.pointVersion = w.writeVersion;
-          n->entry.rangeVersion = p->entry.rangeVersion;
+          n->entry.rangeVersion =
+              p != nullptr ? p->entry.rangeVersion : oldestVersion;
         } else {
           n->entry.pointVersion =
               std::max(n->entry.pointVersion, w.writeVersion);
@@ -1078,31 +1028,6 @@ __attribute__((__visibility__("default"))) void ConflictSet_destroy(void *cs) {
 
 namespace {
 
-void printLogical(std::string &result, Node *node) {
-  Arena arena;
-  for (Node *iter = node; iter != nullptr;) {
-    auto *next = nextLogical(iter);
-    std::string key;
-    for (uint8_t c : getSearchPath(arena, iter)) {
-      key += "x";
-      key += "0123456789abcdef"[c / 16];
-      key += "0123456789abcdef"[c % 16];
-    }
-    if (iter->entry.pointVersion == iter->entry.rangeVersion) {
-      result += key + " -> " + std::to_string(iter->entry.pointVersion) + "\n";
-    } else {
-      result += key + " -> " + std::to_string(iter->entry.pointVersion) + "\n";
-      if (next == nullptr || (getSearchPath(arena, next) !=
-                              (std::string(getSearchPath(arena, iter)) +
-                               std::string("\x00", 1)))) {
-        result +=
-            key + "x00 -> " + std::to_string(iter->entry.rangeVersion) + "\n";
-      }
-    }
-    iter = next;
-  }
-}
-
 [[maybe_unused]] void debugPrintDot(FILE *file, Node *node) {
 
   struct DebugDotPrinter {
@@ -1189,25 +1114,13 @@ void checkParentPointers(Node *node, bool &success) {
   return total;
 }
 
-bool checkCorrectness(Node *node, ReferenceImpl &refImpl) {
+bool checkCorrectness(Node *node) {
   bool success = true;
 
   checkParentPointers(node, success);
   checkMaxVersion(node, success);
   checkEntriesExist(node, success);
 
-  std::string logicalMap;
-  std::string referenceLogicalMap;
-  printLogical(logicalMap, node);
-  refImpl.printLogical(referenceLogicalMap);
-  if (logicalMap != referenceLogicalMap) {
-    fprintf(stderr,
-            "Logical map not equal to reference logical map.\n\nActual:\n"
-            "%s\nExpected:\n%s\n",
-            logicalMap.c_str(), referenceLogicalMap.c_str());
-    success = false;
-  }
-
   return success;
 }
 
@@ -1285,7 +1198,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
 #if DEBUG_VERBOSE && !defined(NDEBUG)
     fprintf(stderr, "Check correctness\n");
 #endif
-    bool success = checkCorrectness(driver.cs.root, driver.refImpl);
+    bool success = checkCorrectness(driver.cs.root);
     if (!success) {
       debugPrintDot(stdout, driver.cs.root);
       fflush(stdout);

Internal.h

@@ -393,18 +393,6 @@ struct ReferenceImpl {
     this->oldestVersion = oldestVersion;
   }
 
-  void printLogical(std::string &result) {
-    for (const auto &[k, v] : writeVersionMap) {
-      std::string key;
-      for (uint8_t c : k) {
-        key += "x";
-        key += "0123456789abcdef"[c / 16];
-        key += "0123456789abcdef"[c % 16];
-      }
-      result += key + " -> " + std::to_string(v) + "\n";
-    }
-  }
-
   int64_t oldestVersion;
   std::map<std::string, int64_t> writeVersionMap;
 };