eraseTree() accounted for the nodes it releases and the entries it erases in writeContext->accum, but addWrites() immediately calls init(), which
reconstructs the WriteContext and zeroes accum. By the time addWrites()
reported the metrics, the tree reset's accounting was gone, so nodes_released_total / entries_erased_total never showed the one path that
frees an entire tree.
Fix
init() snapshots writeContext.accum before destroying the WriteContext
and restores it after the placement new. It is called both from the
constructor and from the version-window reset, so every reset path keeps the
running totals that addWrites() reports from.
Drop ++writeContext->accum.nodes_released in eraseTree(): release()
already increments that counter for the same node, so it was a double count
that only stayed invisible because init() threw the whole accum away.
The entries_erased increment stays, because release() does not account
for the entry.
Test
ShowMemoryTest.cpp now asserts, right after its version-jump blocks, that the
erased nodes and entries are reported (and counted once):
the small block erases a 2-node, 2-entry tree ("" in the root, "abc" in a
child) and asserts entries_erased_total == 2, nodes_released_total == 2;
the 1000-key block asserts entries_erased_total == 1001 and nodes_released_total >= 1000.
Both assertions fail without the fix (with the buggy code the counter stays 0).
Verification
Full ctest suite passes in Release (8603 tests, incl. valgrind) and in
Debug (5710 tests).
conflict_set_show_memory_test also built and passed with gcc and with -DUSE_64_BIT=1.
The issue's reproduction now reports the version-jump block as nodes_released_total = 5, entries_erased_total = 5 (the tree at the jump
holds 5 nodes and 5 entries including the root's "" entry); it reported
0/0 before. nodes_allocated_total for that workload moved from 10 to 11
because the rootParent allocated in the constructor is now carried across
the constructor's own init() instead of being discarded.
Not addressed here (separate pre-existing gap)
WriteContext::releaseDeferred() calls the per-type allocator's releaseDeferred() directly, bypassing WriteContext::release<T>(), so nodes
replaced by grow/shrink (and freed at the end of addWrites()) are never
counted in nodes_released_total. That is why the erased tree in the issue's
reproduction has 5 physical nodes rather than the 7 suggested there, and it
means nodes_allocated_total - nodes_released_total still overestimates live
nodes for workloads that grow/shrink nodes. Worth a follow-up if you want that
gauge to be exact.
Closes #100
## Root cause
`eraseTree()` accounted for the nodes it releases and the entries it erases in
`writeContext->accum`, but `addWrites()` immediately calls `init()`, which
reconstructs the `WriteContext` and zeroes `accum`. By the time `addWrites()`
reported the metrics, the tree reset's accounting was gone, so
`nodes_released_total` / `entries_erased_total` never showed the one path that
frees an entire tree.
## Fix
* `init()` snapshots `writeContext.accum` before destroying the `WriteContext`
and restores it after the placement new. It is called both from the
constructor and from the version-window reset, so every reset path keeps the
running totals that `addWrites()` reports from.
* Drop `++writeContext->accum.nodes_released` in `eraseTree()`: `release()`
already increments that counter for the same node, so it was a double count
that only stayed invisible because `init()` threw the whole `accum` away.
The `entries_erased` increment stays, because `release()` does not account
for the entry.
## Test
`ShowMemoryTest.cpp` now asserts, right after its version-jump blocks, that the
erased nodes and entries are reported (and counted once):
* the small block erases a 2-node, 2-entry tree (`""` in the root, `"abc"` in a
child) and asserts `entries_erased_total == 2`, `nodes_released_total == 2`;
* the 1000-key block asserts `entries_erased_total == 1001` and
`nodes_released_total >= 1000`.
Both assertions fail without the fix (with the buggy code the counter stays 0).
## Verification
* Full `ctest` suite passes in Release (8603 tests, incl. valgrind) and in
Debug (5710 tests).
* `conflict_set_show_memory_test` also built and passed with gcc and with
`-DUSE_64_BIT=1`.
* The issue's reproduction now reports the version-jump block as
`nodes_released_total = 5`, `entries_erased_total = 5` (the tree at the jump
holds 5 nodes and 5 entries including the root's `""` entry); it reported
0/0 before. `nodes_allocated_total` for that workload moved from 10 to 11
because the `rootParent` allocated in the constructor is now carried across
the constructor's own `init()` instead of being discarded.
## Not addressed here (separate pre-existing gap)
`WriteContext::releaseDeferred()` calls the per-type allocator's
`releaseDeferred()` directly, bypassing `WriteContext::release<T>()`, so nodes
replaced by grow/shrink (and freed at the end of `addWrites()`) are never
counted in `nodes_released_total`. That is why the erased tree in the issue's
reproduction has 5 physical nodes rather than the 7 suggested there, and it
means `nodes_allocated_total - nodes_released_total` still overestimates live
nodes for workloads that grow/shrink nodes. Worth a follow-up if you want that
gauge to be exact.
CI / test (arm64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 22, https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst, debug-arm64, ubuntu-latest-arm64) (pull_request) Successful in 3m56s
CI / test (amd64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 21, https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst, debug, ubuntu-latest-amd64) (pull_request) Successful in 4m2s
`addWrites()` tears down the whole tree with `eraseTree()` when the version
window is exceeded, then calls `init()`, which reconstructs the `WriteContext`
and zeroes `accum`. Everything `eraseTree()` accounted for was discarded
before the metrics were reported, so `nodes_released_total` and
`entries_erased_total` never reflected the one path that frees an entire tree.
`init()` now carries `accum` across the reconstruction of `writeContext`, so
the accounting done before the reset still gets reported by `addWrites()`.
`eraseTree()` also incremented `accum.nodes_released` itself before calling
`release()`, which increments the same counter, double counting every node it
released. That was only invisible because `init()` zeroed `accum`. Drop the
redundant increment and keep the entry accounting, which `release()` does not
do.
Extend ShowMemoryTest.cpp to check that a version jump reports the nodes and
entries it erases.
Closes#100
All checks were successful
CI / pre-commit (pull_request) Successful in 2m18s
CI / test (arm64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 22, https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst, debug-arm64, ubuntu-latest-arm64) (pull_request) Successful in 3m56s
CI / test (amd64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 21, https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst, debug, ubuntu-latest-amd64) (pull_request) Successful in 4m2s
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #100
Root cause
eraseTree()accounted for the nodes it releases and the entries it erases inwriteContext->accum, butaddWrites()immediately callsinit(), whichreconstructs the
WriteContextand zeroesaccum. By the timeaddWrites()reported the metrics, the tree reset's accounting was gone, so
nodes_released_total/entries_erased_totalnever showed the one path thatfrees an entire tree.
Fix
init()snapshotswriteContext.accumbefore destroying theWriteContextand restores it after the placement new. It is called both from the
constructor and from the version-window reset, so every reset path keeps the
running totals that
addWrites()reports from.++writeContext->accum.nodes_releasedineraseTree():release()already increments that counter for the same node, so it was a double count
that only stayed invisible because
init()threw the wholeaccumaway.The
entries_erasedincrement stays, becauserelease()does not accountfor the entry.
Test
ShowMemoryTest.cppnow asserts, right after its version-jump blocks, that theerased nodes and entries are reported (and counted once):
""in the root,"abc"in achild) and asserts
entries_erased_total == 2,nodes_released_total == 2;entries_erased_total == 1001andnodes_released_total >= 1000.Both assertions fail without the fix (with the buggy code the counter stays 0).
Verification
ctestsuite passes in Release (8603 tests, incl. valgrind) and inDebug (5710 tests).
conflict_set_show_memory_testalso built and passed with gcc and with-DUSE_64_BIT=1.nodes_released_total = 5,entries_erased_total = 5(the tree at the jumpholds 5 nodes and 5 entries including the root's
""entry); it reported0/0 before.
nodes_allocated_totalfor that workload moved from 10 to 11because the
rootParentallocated in the constructor is now carried acrossthe constructor's own
init()instead of being discarded.Not addressed here (separate pre-existing gap)
WriteContext::releaseDeferred()calls the per-type allocator'sreleaseDeferred()directly, bypassingWriteContext::release<T>(), so nodesreplaced by grow/shrink (and freed at the end of
addWrites()) are nevercounted in
nodes_released_total. That is why the erased tree in the issue'sreproduction has 5 physical nodes rather than the 7 suggested there, and it
means
nodes_allocated_total - nodes_released_totalstill overestimates livenodes for workloads that grow/shrink nodes. Worth a follow-up if you want that
gauge to be exact.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.