From b671f93e5a63687b6f86b31c8f7c524c44f1de6a Mon Sep 17 00:00:00 2001 From: Weaselbot Date: Tue, 10 Nov 2026 15:37:54 -0500 Subject: [PATCH] Document that SHOW_MEMORY assumes a single live ConflictSet The testing-only cross-check in `internal_addWrites` and `internal_setOldestVersion` compares `impl->totalBytes`, which only accounts for the allocations made during one instance's own API calls, against `mallocBytes`, which is process-wide. The two are only equal while a single ConflictSet uses `safe_malloc`, so a SHOW_MEMORY build aborts on perfectly valid programs that keep two instances alive (the repo's own script tests do exactly that). Keeping several ConflictSets alive is supported and does not need the instrumentation fixed for it, so just say so where the invariant lives: on the process-wide counters in `Internal.h`, and at each of the four cross-checks in ConflictSet.cpp and SkipList.cpp. ShowMemoryTest.cpp's header comment and the CMakeLists comment for the test target now point at that explanation instead of stating the restriction without a reason. Closes #97 --- CMakeLists.txt | 2 +- ConflictSet.cpp | 8 ++++++-- Internal.h | 8 ++++++++ ShowMemoryTest.cpp | 5 +++-- SkipList.cpp | 6 ++++++ 5 files changed, 24 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c22adcc..bd290ee 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -394,7 +394,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING) 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 + # compile with. It assumes a single live ConflictSet (see Internal.h). add_executable(conflict_set_show_memory_test ConflictSet.cpp ShowMemoryTest.cpp) target_compile_options(conflict_set_show_memory_test PRIVATE ${TEST_FLAGS}) diff --git a/ConflictSet.cpp b/ConflictSet.cpp index 3273dc2..a1b6dc9 100644 --- a/ConflictSet.cpp +++ b/ConflictSet.cpp @@ -5546,7 +5546,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. + // Testing-only cross-check of the accounting. `mallocBytes` is process-wide, + // while `impl->totalBytes` only covers this instance, so this holds only + // while this is the only live ConflictSet (see Internal.h). if (impl->totalBytes != mallocBytes) { abort(); // GCOVR_EXCL_LINE } @@ -5559,7 +5561,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. + // Testing-only cross-check of the accounting. `mallocBytes` is process-wide, + // while `impl->totalBytes` only covers this instance, so this holds only + // while this is the only live ConflictSet (see Internal.h). if (impl->totalBytes != mallocBytes) { abort(); // GCOVR_EXCL_LINE } diff --git a/Internal.h b/Internal.h index f2b2ea8..54d9a56 100644 --- a/Internal.h +++ b/Internal.h @@ -96,6 +96,14 @@ static_assert(std::is_trivial_v); // GCOVR_EXCL_START #if SHOW_MEMORY +// Process-wide by design: `safe_malloc`/`safe_free` update these for every +// allocation in the process, no matter which ConflictSet made it. An instance's +// `totalBytes`, in contrast, only accounts for the allocations made during its +// own API calls, so the SHOW_MEMORY cross-check of the two in +// `internal_addWrites`/`internal_setOldestVersion` only holds while a single +// ConflictSet is alive. Running several at once is supported by the library; +// SHOW_MEMORY just doesn't instrument it. `peakMallocBytes` is process-wide +// too, but as a peak it remains meaningful with several instances. inline int64_t mallocBytes = 0; inline int64_t peakMallocBytes = 0; // Defined in ConflictSet.cpp diff --git a/ShowMemoryTest.cpp b/ShowMemoryTest.cpp index 440cf7f..3e3c879 100644 --- a/ShowMemoryTest.cpp +++ b/ShowMemoryTest.cpp @@ -1,8 +1,9 @@ // 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. +// by TEST_FLAGS. Only one ConflictSet may exist at a time: the accounting is +// process-wide, while the cross-check compares it against a single instance's +// totals (see the comment on `mallocBytes` in Internal.h). #include "ConflictSet.h" #include "Internal.h" diff --git a/SkipList.cpp b/SkipList.cpp index d64d550..4aba838 100644 --- a/SkipList.cpp +++ b/SkipList.cpp @@ -915,6 +915,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. `mallocBytes` is process-wide, + // while `impl->totalBytes` only covers this instance, so this holds only + // while this is the only live ConflictSet (see Internal.h). if (impl->totalBytes != mallocBytes) { abort(); } @@ -927,6 +930,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. `mallocBytes` is process-wide, + // while `impl->totalBytes` only covers this instance, so this holds only + // while this is the only live ConflictSet (see Internal.h). if (impl->totalBytes != mallocBytes) { abort(); }