Document and test thread safety properties

Closes #2
De-templatize kUseFreeList
2024-03-19 16:27:24 -07:00 · 2024-03-19 15:12:31 -07:00 · 2024-03-19 15:05:34 -07:00 · 2024-03-19 15:01:13 -07:00 · 2024-03-19 14:34:59 -07:00
6 changed files with 130 additions and 90 deletions
--- a/.clangd
+++ b/.clangd
@@ -1,2 +1,2 @@
 CompileFlags:
-  Add: [-DENABLE_MAIN, -UNDEBUG, -DENABLE_FUZZ, -fexceptions]
+  Add: [-DENABLE_MAIN, -UNDEBUG, -DENABLE_FUZZ, -DTHREAD_TEST, -fexceptions]
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -167,6 +167,21 @@ if(BUILD_TESTING)
    add_test(NAME conflict_set_fuzz_${hash} COMMAND fuzz_driver ${TEST})
  endforeach()

+  # tsan
+
+  if(NOT CMAKE_CROSSCOMPILING)
+    add_executable(tsan_driver ConflictSet.cpp FuzzTestDriver.cpp)
+    target_compile_options(tsan_driver PRIVATE ${TEST_FLAGS} -fsanitize=thread)
+    target_link_options(tsan_driver PRIVATE -fsanitize=thread)
+    target_compile_definitions(tsan_driver PRIVATE ENABLE_FUZZ THREAD_TEST)
+    target_include_directories(tsan_driver
+                               PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+    foreach(TEST ${CORPUS_TESTS})
+      get_filename_component(hash ${TEST} NAME)
+      add_test(NAME conflict_set_tsan_${hash} COMMAND tsan_driver ${TEST})
+    endforeach()
+  endif()
+
  add_executable(driver TestDriver.cpp)
  target_compile_options(driver PRIVATE ${TEST_FLAGS})
  target_link_libraries(driver PRIVATE ${PROJECT_NAME})
--- a/ConflictSet.cpp
+++ b/ConflictSet.cpp
@@ -998,17 +998,17 @@ Node *nextLogical(Node *node) {

 // Invalidates `self`, replacing it with a node of at least capacity.
 // Does not return nodes to freelists when kUseFreeList is false.
-template <bool kUseFreeList>
 void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
                                NodeAllocators *allocators,
-                                ConflictSet::Impl *impl) {
+                                ConflictSet::Impl *impl,
+                                const bool kUseFreeList) {
  switch (self->getType()) {
  case Type_Node0: {
    auto *self0 = (Node0 *)self;
    auto *newSelf = allocators->node0.allocate(capacity);
    newSelf->copyChildrenAndKeyFrom(*self0);
    getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
      allocators->node0.release(self0);
    } else {
      removeNode(self0);
@@ -1021,7 +1021,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
    auto *newSelf = allocators->node3.allocate(capacity);
    newSelf->copyChildrenAndKeyFrom(*self3);
    getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
      allocators->node3.release(self3);
    } else {
      removeNode(self3);
@@ -1034,7 +1034,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
    auto *newSelf = allocators->node16.allocate(capacity);
    newSelf->copyChildrenAndKeyFrom(*self16);
    getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
      allocators->node16.release(self16);
    } else {
      removeNode(self16);
@@ -1047,7 +1047,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
    auto *newSelf = allocators->node48.allocate(capacity);
    newSelf->copyChildrenAndKeyFrom(*self48);
    getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
      allocators->node48.release(self48);
    } else {
      removeNode(self48);
@@ -1060,7 +1060,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
    auto *newSelf = allocators->node256.allocate(capacity);
    newSelf->copyChildrenAndKeyFrom(*self256);
    getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
      allocators->node256.release(self256);
    } else {
      removeNode(self256);
@@ -1083,11 +1083,9 @@ void maybeDecreaseCapacity(Node *&self, NodeAllocators *allocators,
  if (self->getCapacity() <= maxCapacity) {
    return;
  }
-  freeAndMakeCapacityAtLeast</*kUseFreeList*/ false>(self, maxCapacity,
-                                                     allocators, impl);
+  freeAndMakeCapacityAtLeast(self, maxCapacity, allocators, impl, false);
 }

-template <Type kType>
 void maybeDownsize(Node *self, NodeAllocators *allocators,
                   ConflictSet::Impl *impl, Node *&dontInvalidate) {

@@ -1095,9 +1093,7 @@ void maybeDownsize(Node *self, NodeAllocators *allocators,
  fprintf(stderr, "maybeDownsize: %s\n", getSearchPathPrintable(self).c_str());
 #endif

-  assert(self->getType() == kType);
-  static_assert(kType != Type_Node0);
-  switch (kType) {
+  switch (self->getType()) {
  case Type_Node0:           // GCOVR_EXCL_LINE
    __builtin_unreachable(); // GCOVR_EXCL_LINE
  case Type_Node3: {
@@ -1113,8 +1109,7 @@ void maybeDownsize(Node *self, NodeAllocators *allocators,

      if (minCapacity > child->getCapacity()) {
        const bool update = child == dontInvalidate;
-        freeAndMakeCapacityAtLeast</*kUseFreeList*/ true>(child, minCapacity,
-                                                          allocators, impl);
+        freeAndMakeCapacityAtLeast(child, minCapacity, allocators, impl, true);
        if (update) {
          dontInvalidate = child;
        }
@@ -1201,24 +1196,7 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,

  if (self->numChildren != 0) {
    const bool update = result == dontInvalidate;
-    switch (self->getType()) {
-    case Type_Node0:           // GCOVR_EXCL_LINE
-      __builtin_unreachable(); // GCOVR_EXCL_LINE
-    case Type_Node3:
-      maybeDownsize<Type_Node3>(self, allocators, impl, result);
-      break;
-    case Type_Node16:
-      maybeDownsize<Type_Node16>(self, allocators, impl, result);
-      break;
-    case Type_Node48:
-      maybeDownsize<Type_Node48>(self, allocators, impl, result);
-      break;
-    case Type_Node256:
-      maybeDownsize<Type_Node256>(self, allocators, impl, result);
-      break;
-    default:                   // GCOVR_EXCL_LINE
-      __builtin_unreachable(); // GCOVR_EXCL_LINE
-    }
+    maybeDownsize(self, allocators, impl, result);
    if (update) {
      dontInvalidate = result;
    }
@@ -1244,11 +1222,6 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,

    --parent->numChildren;
    assert(parent->numChildren > 0 || parent->entryPresent);
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node3>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
  } break;
  case Type_Node16: {
    auto *parent16 = static_cast<Node16 *>(parent);
@@ -1265,13 +1238,6 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,

    // By kMinChildrenNode16
    assert(parent->numChildren > 0);
-
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node16>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
-
  } break;
  case Type_Node48: {
    auto *parent48 = static_cast<Node48 *>(parent);
@@ -1292,12 +1258,6 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,

    // By kMinChildrenNode48
    assert(parent->numChildren > 0);
-
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node48>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
  } break;
  case Type_Node256: {
    auto *parent256 = static_cast<Node256 *>(parent);
@@ -1309,16 +1269,17 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,
    // By kMinChildrenNode256
    assert(parent->numChildren > 0);

-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node256>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
  } break;
  default:                   // GCOVR_EXCL_LINE
    __builtin_unreachable(); // GCOVR_EXCL_LINE
  }

+  const bool update = result == dontInvalidate;
+  maybeDownsize(parent, allocators, impl, result);
+  if (update) {
+    dontInvalidate = result;
+  }
+
  return result;
 }

@@ -1698,6 +1659,9 @@ bool checkRangeStartsWith(Node *n, std::span<const uint8_t> key, int begin,
      goto downLeftSpine;
    } else {
      n = nextSibling(n);
+      if (n == nullptr) {
+        return true;
+      }
      goto downLeftSpine;
    }
  }
@@ -1715,6 +1679,9 @@ bool checkRangeStartsWith(Node *n, std::span<const uint8_t> key, int begin,
        goto downLeftSpine;
      } else {
        n = nextSibling(n);
+        if (n == nullptr) {
+          return true;
+        }
        goto downLeftSpine;
      }
    }
@@ -1732,9 +1699,6 @@ bool checkRangeStartsWith(Node *n, std::span<const uint8_t> key, int begin,
  __builtin_unreachable(); // GCOVR_EXCL_LINE

 downLeftSpine:
-  if (n == nullptr) {
-    return true;
-  }
  for (;;) {
    if (n->entryPresent) {
      return n->entry.rangeVersion <= readVersion;
@@ -1802,6 +1766,10 @@ struct CheckRangeLeftSide {
          return true;
        } else {
          n = nextSibling(n);
+          if (n == nullptr) {
+            ok = true;
+            return true;
+          }
          return downLeftSpine();
        }
      }
@@ -1829,6 +1797,10 @@ struct CheckRangeLeftSide {
            return true;
          } else {
            n = nextSibling(n);
+            if (n == nullptr) {
+              ok = true;
+              return true;
+            }
            return downLeftSpine();
          }
        }
@@ -1865,10 +1837,6 @@ struct CheckRangeLeftSide {

  bool downLeftSpine() {
    phase = DownLeftSpine;
-    if (n == nullptr) {
-      ok = true;
-      return true;
-    }
    return false;
  }
 };
--- a/FuzzTestDriver.cpp
+++ b/FuzzTestDriver.cpp
@@ -2,15 +2,25 @@
 #include <cstdint>
 #include <fstream>
 #include <sstream>
+#include <thread>

 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);

 int main(int argc, char **argv) {
-  for (int i = 1; i < argc; ++i) {
-    std::ifstream t(argv[i], std::ios::binary);
-    std::stringstream buffer;
-    buffer << t.rdbuf();
-    auto str = buffer.str();
-    LLVMFuzzerTestOneInput((const uint8_t *)str.data(), str.size());
-  }
+  auto doTest = [&]() {
+    for (int i = 1; i < argc; ++i) {
+      std::ifstream t(argv[i], std::ios::binary);
+      std::stringstream buffer;
+      buffer << t.rdbuf();
+      auto str = buffer.str();
+      LLVMFuzzerTestOneInput((const uint8_t *)str.data(), str.size());
+    }
+  };
+#ifdef THREAD_TEST
+  std::thread thread2{doTest};
+#endif
+  doTest();
+#ifdef THREAD_TEST
+  thread2.join();
+#endif
 }
--- a/Internal.h
+++ b/Internal.h
@@ -10,10 +10,12 @@
 #include <cstdlib>
 #include <cstring>
 #include <inttypes.h>
+#include <latch>
 #include <map>
 #include <set>
 #include <span>
 #include <string>
+#include <thread>
 #include <unordered_set>
 #include <utility>
 #include <vector>
@@ -652,32 +654,60 @@ template <class ConflictSetImpl> struct TestDriver {
      auto *results2 =
          new (arena) ConflictSet::Result[numPointReads + numRangeReads];

+#ifdef THREAD_TEST
+      auto *results3 =
+          new (arena) ConflictSet::Result[numPointReads + numRangeReads];
+      std::latch ready{1};
+      std::thread thread2{[&]() {
+        ready.count_down();
+        cs.check(reads, results3, numPointReads + numRangeReads);
+      }};
+      ready.wait();
+#endif
+
      CALLGRIND_START_INSTRUMENTATION;
      cs.check(reads, results1, numPointReads + numRangeReads);
      CALLGRIND_STOP_INSTRUMENTATION;

      refImpl.check(reads, results2, numPointReads + numRangeReads);
-      for (int i = 0; i < numPointReads + numRangeReads; ++i) {
-        if (results1[i] != results2[i]) {
-          if (reads[i].end.len == 0) {
-            fprintf(stderr,
-                    "Expected %s, got %s for read of {%s} at version %" PRId64
-                    "\n",
-                    resultToStr(results2[i]), resultToStr(results1[i]),
-                    printable(reads[i].begin).c_str(), reads[i].readVersion);
-          } else {
-            fprintf(
-                stderr,
-                "Expected %s, got %s for read of [%s, %s) at version %" PRId64
-                "\n",
-                resultToStr(results2[i]), resultToStr(results1[i]),
-                printable(reads[i].begin).c_str(),
-                printable(reads[i].end).c_str(), reads[i].readVersion);
+
+      auto compareResults = [reads](ConflictSet::Result *results1,
+                                    ConflictSet::Result *results2, int count) {
+        for (int i = 0; i < count; ++i) {
+          if (results1[i] != results2[i]) {
+            if (reads[i].end.len == 0) {
+              fprintf(stderr,
+                      "Expected %s, got %s for read of {%s} at version %" PRId64
+                      "\n",
+                      resultToStr(results2[i]), resultToStr(results1[i]),
+                      printable(reads[i].begin).c_str(), reads[i].readVersion);
+            } else {
+              fprintf(
+                  stderr,
+                  "Expected %s, got %s for read of [%s, %s) at version %" PRId64
+                  "\n",
+                  resultToStr(results2[i]), resultToStr(results1[i]),
+                  printable(reads[i].begin).c_str(),
+                  printable(reads[i].end).c_str(), reads[i].readVersion);
+            }
+            return false;
          }
-          ok = false;
-          return true;
        }
+        return true;
+      };
+
+      if (!compareResults(results1, results2, numPointReads + numRangeReads)) {
+        ok = false;
+        return true;
      }
+
+#ifdef THREAD_TEST
+      thread2.join();
+      if (!compareResults(results3, results2, numPointReads + numRangeReads)) {
+        ok = false;
+        return true;
+      }
+#endif
    }
    return false;
  }
--- a/include/ConflictSet.h
+++ b/include/ConflictSet.h
@@ -19,7 +19,15 @@ limitations under the License.
 #include <stdint.h>

 #ifdef __cplusplus
-
+/** A data structure for optimistic concurrency control on ranges of
+ * bitwise-lexicographically-ordered keys.
+ *
+ * Thread safety:
+ *   - It's safe to operate on two different ConflictSets in two different
+ * threads concurrently
+ *   - It's safe to have multiple threads operating on the same ConflictSet
+ * concurrently if and only if all threads only call `check`.
+ */
 struct __attribute__((__visibility__("default"))) ConflictSet {
  enum Result {
    /** The result of a check which does not intersect any conflicting writes */
@@ -92,6 +100,15 @@ private:

 #else

+/** A data structure for optimistic concurrency control on ranges of
+ * bitwise-lexicographically-ordered keys.
+ *
+ * Thread safety:
+ *   - It's safe to operate on two different ConflictSets in two different
+ * threads concurrently
+ *   - It's safe to have multiple threads operating on the same ConflictSet
+ * concurrently if and only if all threads only call `check`.
+ */
 typedef struct ConflictSet ConflictSet;

 typedef enum {
Author	SHA1	Message	Date
Andrew Noyes	aa6f237d50	Document and test thread safety properties Some checks reported errors Tests / Clang total: 1096, passed: 1096 Details Clang \|Total\|New\|Outstanding\|Fixed\|Trend \|:-:\|:-:\|:-:\|:-:\|:-: \|0\|0\|0\|0\|:clap: Details Tests / Release [gcc] total: 1096, passed: 1096 Details GNU C Compiler (gcc) \|Total\|New\|Outstanding\|Fixed\|Trend \|:-:\|:-:\|:-:\|:-:\|:-: \|0\|0\|0\|0\|:clap: Details Tests / Release [gcc,aarch64] total: 824, passed: 824 Details weaselab/conflict-set/pipeline/head Something is wrong with the build of this commit Details Closes #2	2024-03-19 16:27:24 -07:00
Andrew Noyes	becfd25139	De-templatize kUseFreeList This results in smaller code. This is part of the "let the compiler be in charge of inlining decisions" theme.	2024-03-19 15:12:31 -07:00
Andrew Noyes	d78b36821b	Remove more redundant nullptr checks	2024-03-19 15:05:34 -07:00
Andrew Noyes	ce79b47fbe	Revert `303b368fc5` This is a code-size / speed tradeoff. Maybe it's a good idea here, but it's a bit weird to do this in some places and not others (there are many places we can avoid switching on type this way). The compiler can inline and then dead code eliminate to achieve the same effect, so we'll just let the compiler be in charge of inlining decisions.	2024-03-19 15:01:13 -07:00
Andrew Noyes	727b7e642a	Save more redundant nullptr checks	2024-03-19 14:34:59 -07:00