From 67c8ca8f3a5d4d8ee5d8493ce7a32808574bae2e Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Tue, 14 May 2024 17:44:01 -0700 Subject: [PATCH] Fix two node-copying bugs in update --- Facade.h | 1 - FacadeTest.cpp | 35 ++++++++++++++++++++++++++++++++--- PrintMutation.h | 29 +++++++++++++++++++++++++---- VersionedMap.cpp | 26 +++++++++++++++++--------- 4 files changed, 74 insertions(+), 17 deletions(-) diff --git a/Facade.h b/Facade.h index c71d121..ee89ab1 100644 --- a/Facade.h +++ b/Facade.h @@ -136,7 +136,6 @@ struct Facade { int64_t getVersion() const { return versioned.getVersion(); } int64_t getOldestVersion() const { return versioned.getOldestVersion(); } -private: std::vector> unversionedRead(const String &begin, const String &end, int &limit, diff --git a/FacadeTest.cpp b/FacadeTest.cpp index f03136d..4aebe89 100644 --- a/FacadeTest.cpp +++ b/FacadeTest.cpp @@ -1,4 +1,5 @@ #include "Facade.h" +#include "PrintMutation.h" #include "VersionedMap.h" #include @@ -24,6 +25,8 @@ weaselab::VersionedMap::Mutation clear(weaselab::VersionedMap::Key begin, return {begin.p, end.p, begin.len, end.len, weaselab::VersionedMap::Clear}; } +void breakpoint_me() {} + int main() { Facade f(0); int64_t version = 1; @@ -42,13 +45,14 @@ int main() { } for (int64_t i = 0; i < version; ++i) { printf("--- version %" PRId64 " ---\n", i); - auto result = f.viewAt(i).rangeRead("a"_s, "d"_s, 10, false); + printf("merged:\n"); + auto result = f.viewAt(i).rangeRead("a"_s, "z"_s, 100, false); for (const auto &[k, v] : result) { for (auto c : k) { if (isprint(c)) { printf("%c", c); } else { - printf("0x%02x", c); + printf("x%02x", c); } } printf(" -> "); @@ -56,10 +60,35 @@ int main() { if (isprint(c)) { printf("%c", c); } else { - printf("0x%02x", c); + printf("x%02x", c); } } printf("\n"); } + printf("unversioned:\n"); + for (const auto &[k, v] : f.unversioned) { + for (auto c : k) { + if (isprint(c)) { + printf("%c", c); + } else { + printf("x%02x", c); + } + } + printf(" -> "); + for (auto c : v) { + if (isprint(c)) { + printf("%c", c); + } else { + printf("x%02x", c); + } + } + printf("\n"); + } + breakpoint_me(); + printf("versioned:\n"); + for (auto iter = f.versioned.begin(i), end = f.versioned.end(i); + iter != end; ++iter) { + printMutation(*iter); + } } } \ No newline at end of file diff --git a/PrintMutation.h b/PrintMutation.h index 2803d73..ec39215 100644 --- a/PrintMutation.h +++ b/PrintMutation.h @@ -1,6 +1,7 @@ #pragma once #include "VersionedMap.h" +#include #include #include @@ -10,22 +11,42 @@ printMutation(const weaselab::VersionedMap::Iterator::VersionedMutation &m) { case weaselab::VersionedMap::Set: printf("set "); for (int i = 0; i < m.param1Len; ++i) { - printf("x%02x", m.param1[i]); + auto c = m.param1[i]; + if (isprint(c)) { + printf("%c", c); + } else { + printf("x%02x", c); + } } printf(" -> '"); for (int i = 0; i < m.param2Len; ++i) { - printf("x%02x", m.param2[i]); + auto c = m.param2[i]; + if (isprint(c)) { + printf("%c", c); + } else { + printf("x%02x", c); + } } printf("' @ %" PRId64 "\n", m.version); break; case weaselab::VersionedMap::Clear: printf("clear ["); for (int i = 0; i < m.param1Len; ++i) { - printf("x%02x", m.param1[i]); + auto c = m.param1[i]; + if (isprint(c)) { + printf("%c", c); + } else { + printf("x%02x", c); + } } printf(", "); for (int i = 0; i < m.param2Len; ++i) { - printf("x%02x", m.param2[i]); + auto c = m.param2[i]; + if (isprint(c)) { + printf("%c", c); + } else { + printf("x%02x", c); + } } printf(") @ %" PRId64 "\n", m.version); break; diff --git a/VersionedMap.cpp b/VersionedMap.cpp index 8589c3c..e009279 100644 --- a/VersionedMap.cpp +++ b/VersionedMap.cpp @@ -638,7 +638,8 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl { auto &c = mm.base[copy]; c.entry = n.entry->addref(); c.pointer[which] = child; - c.pointer[!which] = n.pointer[!which]; + c.pointer[!which] = + this->child(node, !which, latestVersion); c.updated.store(false, std::memory_order_relaxed); c.updateVersion = version; assert(copy == 0 || copy >= kMinAddressable); @@ -649,11 +650,12 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl { // The reason these aren't data races is that concurrent readers are // reading < `version` if (updated && n.replacedPointer != which) { + auto result = doCopy(); // We can't update n.replacedPointer without introducing a data race // (unless we packed it into the atomic?) so we copy. pointer[2] becomes // unreachable, but need to tell the garbage collector. n.pointer[2] = 0; - return doCopy(); + return result; } else if (updated) { n.pointer[2] = child; } else { @@ -808,12 +810,13 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl { void remove(Finger &finger) { #ifndef NDEBUG - uint32_t expected; + Entry *expected; { Finger copy; finger.copyTo(copy); move(copy, latestVersion, true); - expected = copy.searchPathSize() > 0 ? copy.backNode() : 0; + expected = + copy.searchPathSize() > 0 ? mm.base[copy.backNode()].entry : nullptr; } #endif @@ -850,7 +853,7 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl { // propagate up the search path, all the way to the root since we may have // more rotations to do even if an update doesn't change a node pointer auto node = finger.backNode(); - const auto oldSize = finger.searchPathSize(); + const auto oldSize = finger.searchPathSize() - (node == 0); for (;;) { if (finger.searchPathSize() == 1) { // Made it to the root @@ -873,12 +876,17 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl { finger.push(c, false); } } else { - move(finger, latestVersion, true); + while (finger.searchPathSize() > 1 && finger.backDirection() == true) { + finger.pop(); + } + finger.pop(); } #ifndef NDEBUG if (finger.searchPathSize() > 0) { - assert(finger.backNode() == expected); + assert(mm.base[finger.backNode()].entry == expected); + } else { + assert(expected == nullptr); } #endif } @@ -1342,7 +1350,7 @@ void VersionedMap::Impl::printInOrderHelper(int64_t version, uint32_t node, return; } printInOrderHelper(version, - child(node, false, version), + child(node, true, version), depth + 1); for (int i = 0; i < depth; ++i) { printf(" "); @@ -1360,7 +1368,7 @@ void VersionedMap::Impl::printInOrderHelper(int64_t version, uint32_t node, } printf("\n"); VersionedMap::Impl::printInOrderHelper( - version, child(node, true, version), + version, child(node, false, version), depth + 1); }