diff --git a/CMakeLists.txt b/CMakeLists.txt index 733818c..736483c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -390,6 +390,24 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING) PROPERTIES CXX_STANDARD_REQUIRED ON) add_test(NAME conflict_set_cxx_api_test COMMAND conflict_set_cxx_api_test) + # SHOW_MEMORY instrumentation, which the rest of the test suite doesn't + # compile with + add_executable(conflict_set_show_memory_test ConflictSet.cpp + ShowMemoryTest.cpp) + target_compile_options(conflict_set_show_memory_test PRIVATE ${TEST_FLAGS}) + target_compile_definitions(conflict_set_show_memory_test + PRIVATE SHOW_MEMORY=1) + target_include_directories(conflict_set_show_memory_test + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include) + if(NOT CMAKE_CROSSCOMPILING) + target_compile_options(conflict_set_show_memory_test + PRIVATE -fsanitize=address,undefined) + target_link_options(conflict_set_show_memory_test PRIVATE + -fsanitize=address,undefined) + endif() + add_test(NAME conflict_set_show_memory_test + COMMAND conflict_set_show_memory_test) + # symbol visibility tests if(NOT CMAKE_BUILD_TYPE STREQUAL Debug) if(APPLE) diff --git a/ConflictSet.cpp b/ConflictSet.cpp index 553fb11..326a365 100644 --- a/ConflictSet.cpp +++ b/ConflictSet.cpp @@ -80,17 +80,19 @@ using namespace weaselab; #define assume assert #endif -#if SHOW_MEMORY +// The SHOW_MEMORY instrumentation. Declared unconditionally so that each of +// these has a single definition in every configuration (see the definitions at +// the end of this file), rather than a no-op stub standing in for the real +// thing. void addNode(struct Node *); void removeNode(struct Node *); void addKey(struct Node *); void removeKey(struct Node *); -#else -constexpr void addNode(struct Node *) {} -constexpr void removeNode(struct Node *) {} -constexpr void addKey(struct Node *) {} -constexpr void removeKey(struct Node *) {} -#endif +// Like `removeKey`, but with a caller-provided search path length. Used when +// the node's ancestors may already have been released, which makes walking +// `parent` pointers invalid. +void removeKey(struct Node *, int64_t searchPathLength); +int64_t getSearchPathLength(struct Node *); // ==================== BEGIN IMPLEMENTATION ==================== @@ -307,7 +309,9 @@ struct Node { uint8_t *partialKey(); Type getType() const { return type; } int32_t getCapacity() const { - assert(!releaseDeferred); + // `partialKeyCapacity` is not repurposed by `deferRelease`, so this is also + // valid for nodes with `releaseDeferred` set. It's needed to account for + // the capacity of a deferred node when it's finally released. return partialKeyCapacity; } @@ -2753,10 +2757,39 @@ checkMaxBetweenExclusiveImpl(Node256 *n, int begin, int end, return self; } +#if SHOW_MEMORY +// Length of the key leading to `child`, given the length of the key leading to +// its parent. Consistent with `getSearchPathLength`. +int64_t childSearchPathLength(int64_t parentSearchPathLength, Node *child) { + return parentSearchPathLength + 1 + child->partialKeyLen; +} +#endif + void eraseTree(Node *root, WriteContext *writeContext) { Arena arena; auto toFree = vector(arena); toFree.push_back(root); + // Length of the key leading to each node in `toFree`, on a parallel stack. + // A node is released before its children are visited, so the `parent` + // pointers that `getSearchPathLength` walks are dangling by the time a queued + // node is popped. Carry the lengths along instead. Only the SHOW_MEMORY + // accounting needs them. +#if SHOW_MEMORY + auto searchPathLengths = vector(arena); + searchPathLengths.push_back(getSearchPathLength(root)); +#endif + + // Queue `child` for release, where `parentSearchPathLength` is the length of + // the key leading to its parent. + auto queueChild = [&](Node *child, int64_t parentSearchPathLength) { + toFree.push_back(child); +#if SHOW_MEMORY + searchPathLengths.push_back( + childSearchPathLength(parentSearchPathLength, child)); +#else + (void)parentSearchPathLength; +#endif + }; while (toFree.size() > 0) { auto *n = toFree.back(); @@ -2764,7 +2797,13 @@ void eraseTree(Node *root, WriteContext *writeContext) { writeContext->accum.entries_erased += n->entryPresent; ++writeContext->accum.nodes_released; - removeKey(n); +#if SHOW_MEMORY + const int64_t searchPathLength = searchPathLengths.back(); + searchPathLengths.pop_back(); +#else + const int64_t searchPathLength = 0; +#endif + removeKey(n, searchPathLength); switch (n->getType()) { case Type_Node0: { @@ -2774,28 +2813,39 @@ void eraseTree(Node *root, WriteContext *writeContext) { case Type_Node3: { auto *n3 = static_cast(n); for (int i = 0; i < n3->numChildren; ++i) { - toFree.push_back(n3->children[i]); + queueChild(n3->children[i], searchPathLength); } writeContext->release(n3); } break; case Type_Node16: { auto *n16 = static_cast(n); for (int i = 0; i < n16->numChildren; ++i) { - toFree.push_back(n16->children[i]); + queueChild(n16->children[i], searchPathLength); } writeContext->release(n16); } break; case Type_Node48: { auto *n48 = static_cast(n); for (int i = 0; i < n48->numChildren; ++i) { - toFree.push_back(n48->children[i]); + queueChild(n48->children[i], searchPathLength); } writeContext->release(n48); } break; case Type_Node256: { auto *n256 = static_cast(n); auto *out = toFree.unsafePrepareAppend(n256->numChildren).data(); - n256->bitSet.forEachSet([&](int i) { *out++ = n256->children[i]; }); +#if SHOW_MEMORY + auto *outSearchPathLengths = + searchPathLengths.unsafePrepareAppend(n256->numChildren).data(); +#endif + n256->bitSet.forEachSet([&](int i) { + Node *child = n256->children[i]; + *out++ = child; +#if SHOW_MEMORY + *outSearchPathLengths++ = + childSearchPathLength(searchPathLength, child); +#endif + }); assert(out == toFree.end()); writeContext->release(n256); } break; @@ -5461,8 +5511,9 @@ void internal_addWrites(ConflictSet::Impl *impl, impl->totalBytes += mallocBytesDelta; impl->memory_bytes.set(impl->totalBytes); #if SHOW_MEMORY + // Testing-only cross-check of the accounting. if (impl->totalBytes != mallocBytes) { - abort(); + abort(); // GCOVR_EXCL_LINE } #endif } @@ -5473,8 +5524,9 @@ void internal_setOldestVersion(ConflictSet::Impl *impl, int64_t oldestVersion) { impl->totalBytes += mallocBytesDelta; impl->memory_bytes.set(impl->totalBytes); #if SHOW_MEMORY + // Testing-only cross-check of the accounting. if (impl->totalBytes != mallocBytes) { - abort(); + abort(); // GCOVR_EXCL_LINE } #endif } @@ -5940,6 +5992,14 @@ checkMaxVersion(Node *root, Node *node, InternalVersionT oldestVersion, } // namespace +// The SHOW_MEMORY instrumentation. Each of these functions is defined exactly +// once, with the accounting compiled out when SHOW_MEMORY is disabled. Defining +// them once (rather than defining no-op stubs when SHOW_MEMORY is disabled) +// keeps every configuration of this file reporting each function on the same +// line, which merging coverage from objects compiled with different options +// requires: gcovr, merging the SHOW_MEMORY test with the rest of the build, +// otherwise aborts with "Got function ... on multiple lines". The optimizer +// removes the resulting empty calls. #if SHOW_MEMORY int64_t nodeBytes = 0; @@ -5968,7 +6028,10 @@ int64_t getNodeSize(struct Node *n) { } } +#endif + int64_t getSearchPathLength(Node *n) { +#if SHOW_MEMORY assert(n != nullptr); int64_t result = 0; for (;;) { @@ -5980,9 +6043,14 @@ int64_t getSearchPathLength(Node *n) { n = n->parent; } return result; +#else + (void)n; + return 0; +#endif } void addNode(Node *n) { +#if SHOW_MEMORY nodeBytes += getNodeSize(n); partialCapacityBytes += n->getCapacity(); if (nodeBytes > peakNodeBytes) { @@ -5991,14 +6059,22 @@ void addNode(Node *n) { if (partialCapacityBytes > peakPartialCapacityBytes) { peakPartialCapacityBytes = partialCapacityBytes; } +#else + (void)n; +#endif } void removeNode(Node *n) { +#if SHOW_MEMORY nodeBytes -= getNodeSize(n); partialCapacityBytes -= n->getCapacity(); +#else + (void)n; +#endif } void addKey(Node *n) { +#if SHOW_MEMORY if (!n->entryPresent) { ++totalKeys; keyBytes += getSearchPathLength(n); @@ -6009,15 +6085,36 @@ void addKey(Node *n) { peakKeyBytes = keyBytes; } } +#else + (void)n; +#endif } void removeKey(Node *n) { +#if SHOW_MEMORY if (n->entryPresent) { --totalKeys; keyBytes -= getSearchPathLength(n); } +#else + (void)n; +#endif } +void removeKey(Node *n, int64_t searchPathLength) { +#if SHOW_MEMORY + if (n->entryPresent) { + --totalKeys; + keyBytes -= searchPathLength; + } +#else + (void)n; + (void)searchPathLength; +#endif +} + +#if SHOW_MEMORY + struct __attribute__((visibility("default"))) PeakPrinter { ~PeakPrinter() { printf("--- radix_tree ---\n"); diff --git a/Internal.h b/Internal.h index 7525e0c..f2b2ea8 100644 --- a/Internal.h +++ b/Internal.h @@ -23,7 +23,9 @@ using namespace weaselab; #include #define DEBUG_VERBOSE 0 +#ifndef SHOW_MEMORY #define SHOW_MEMORY 0 +#endif // std::span is not trivially constructible. We want a span that leaves its // members uninitialized for performance reasons. @@ -96,6 +98,11 @@ static_assert(std::is_trivial_v); #if SHOW_MEMORY inline int64_t mallocBytes = 0; inline int64_t peakMallocBytes = 0; +// Defined in ConflictSet.cpp +extern int64_t nodeBytes; +extern int64_t partialCapacityBytes; +extern int64_t totalKeys; +extern int64_t keyBytes; #endif inline thread_local int64_t mallocBytesDelta = 0; diff --git a/ShowMemoryTest.cpp b/ShowMemoryTest.cpp new file mode 100644 index 0000000..440cf7f --- /dev/null +++ b/ShowMemoryTest.cpp @@ -0,0 +1,115 @@ +// Exercises the SHOW_MEMORY instrumentation, which the other tests don't +// compile with (this target passes -DSHOW_MEMORY=1). The instrumentation has +// its own consistency checks, which abort on failure, and asserts are enabled +// by TEST_FLAGS. Only one ConflictSet may exist at a time, since the +// accounting is process-wide. +#include "ConflictSet.h" +#include "Internal.h" + +#include +#include +#include + +using namespace weaselab; + +namespace { + +void writePoint(ConflictSet &cs, const std::string &key, int64_t writeVersion) { + ConflictSet::WriteRange w{{(const uint8_t *)key.data(), int(key.size())}, + {nullptr, 0}}; + cs.addWrites(&w, 1, writeVersion); +} + +ConflictSet::Result checkPoint(const ConflictSet &cs, const std::string &key, + int64_t readVersion) { + ConflictSet::ReadRange r{{(const uint8_t *)key.data(), int(key.size())}, + {nullptr, 0}, + readVersion}; + ConflictSet::Result result; + cs.check(&r, &result, 1); + return result; +} + +// Everything a ConflictSet allocated must have been accounted for and released +// by the time it's destroyed. +void checkFullyReleased() { + assert(nodeBytes == 0); + assert(partialCapacityBytes == 0); + assert(totalKeys == 0); + assert(keyBytes == 0); + assert(mallocBytes == 0); +} + +} // namespace + +int main() { + // Splitting a node replaces it, which defers the release of the old node. + // Releasing a deferred node must account for its capacity, and the key it + // held must be accounted for without walking `parent` pointers of nodes that + // have already been released. + { + ConflictSet cs{0}; + writePoint(cs, "a", 1); + writePoint(cs, "ab", 2); + assert(checkPoint(cs, "a", 0) == ConflictSet::Conflict); + assert(checkPoint(cs, "ab", 0) == ConflictSet::Conflict); + assert(checkPoint(cs, "b", 0) == ConflictSet::Commit); + assert(nodeBytes > 0); + assert(keyBytes > 0); + } + checkFullyReleased(); + + // Exceeding the version window erases the tree mid-workload. + { + ConflictSet cs{0}; + writePoint(cs, "abc", 1); + const int64_t version = int64_t(1) << 40; + writePoint(cs, "abc", version); + assert(checkPoint(cs, "abc", 0) == ConflictSet::TooOld); + assert(checkPoint(cs, "abc", version) == ConflictSet::Commit); + } + checkFullyReleased(); + + // Erase a tree with many nodes, both mid-workload and at destruction. + { + ConflictSet cs{0}; + for (int i = 0; i < 1000; ++i) { + writePoint(cs, "prefix/" + std::to_string(i), i + 1); + } + const int64_t version = int64_t(1) << 40; + writePoint(cs, "prefix/1", version); + assert(checkPoint(cs, "prefix/1", version) == ConflictSet::Commit); + + for (int i = 0; i < 1000; ++i) { + writePoint(cs, "other/" + std::to_string(i), version + i + 1); + } + assert(checkPoint(cs, "other/999", version) == ConflictSet::Conflict); + } + checkFullyReleased(); + + // Grow a node through every node type, so that the accounting for each node + // type's size is exercised, then erase the entries one at a time so that + // nodes are released individually rather than as part of a whole tree. + { + ConflictSet cs{0}; + std::string key(1, '\0'); + for (int i = 0; i < 256; ++i) { + key[0] = char(i); + writePoint(cs, key, i + 1); + } + for (int i = 0; i < 256; ++i) { + key[0] = char(i); + assert(checkPoint(cs, key, i) == ConflictSet::Conflict); + } + + // Garbage collection erases entries that can only fail with TooOld anyway. + // Doing it one version at a time walks the tree, erasing, downsizing and + // releasing nodes as it goes. + for (int i = 0; i < 256; ++i) { + cs.setOldestVersion(i + 1); + } + key[0] = '\0'; + assert(checkPoint(cs, key, 0) == ConflictSet::TooOld); + } + checkFullyReleased(); +}