nodes_released_total / entries_erased_total never count the version-window reset: eraseTree()'s accounting is zeroed by init() (and would double-count once per node)
#100
eraseTree() (the whole-tree teardown used by the version-window reset) counts the nodes it
releases and the entries it erases into writeContext->accum:
but every one of those counts is thrown away before addWrites() reports the metrics,
because addWrites() calls init() immediately after eraseTree(), and init() destroys and
reconstructs the WriteContext, which zeroes accum:
// ConflictSet.cpp:5170-5173
if(oldestExtantVersion<writeVersion-kMaxCorrectVersionWindow)[[unlikely]]{if(writeVersion-kNominalVersionWindow>newestVersionFullPrecision){eraseTree(rootParent->children[0],&writeContext);// 5172
init(writeVersion-kNominalVersionWindow);// 5173
// ConflictSet.cpp:5379-5388
voidinit(int64_toldestVersion){...writeContext.~WriteContext();// 5387
new(&writeContext)WriteContext();// 5388 <-- accum is now all zero
WriteContext() { memset(&accum, 0, sizeof(accum)); } (ConflictSet.cpp:878), so by the time addWrites() reaches nodes_released_total.add(writeContext.accum.nodes_released) (5236) and entries_erased_total.add(writeContext.accum.entries_erased) (5239) both are 0.
Result: the two counters documented as "The total number of physical tree nodes released" and "The total number of entries erased from the tree" (ConflictSet.cpp:5529-5532) never report any
of the work done by the version-window reset — the one path that releases an entire tree in a
single call.
Reproduction
#include<ConflictSet.h>#include<cstdio>#include<cstring>usingnamespaceweaselab;staticvoiddump(ConflictSet&cs,constchar*label){ConflictSet::MetricsV1*m;intcount;cs.getMetricsV1(&m,&count);printf("=== %s\n",label);for(inti=0;i<count;++i)if(strstr(m[i].name,"nodes")||strstr(m[i].name,"entries"))printf(" %-26s = %g\n",m[i].name,m[i].getValue());}staticvoidwritePoints(ConflictSet&cs,int64_t&v){ConflictSet::WriteRangew;for(constchar*k:{"a","b","c","d"}){w.begin={(constuint8_t*)k,1};w.end={nullptr,0};cs.addWrites(&w,1,++v);}}intmain(){{// The same 4 entries, erased by garbage collection -> counted correctly
ConflictSetcs{0};int64_tv=0;writePoints(cs,v);cs.setOldestVersion(v);// gc erases all of them
dump(cs,"erased by setOldestVersion/gc");}{// The same 4 entries, erased by the version-window reset -> not counted
ConflictSetcs{0};int64_tv=0;writePoints(cs,v);ConflictSet::WriteRangew;w.begin={(constuint8_t*)"zzz",3};w.end={nullptr,0};cs.addWrites(&w,1,int64_t(1)<<40);// eraseTree() + init()
dump(cs,"erased by eraseTree/init (version jump)");}}
=== erased by setOldestVersion/gc
nodes_allocated_total = 9
nodes_released_total = 4
entries_inserted_total = 4
entries_erased_total = 4
=== erased by eraseTree/init (version jump)
nodes_allocated_total = 10
nodes_released_total = 0 <-- 7 nodes were released
entries_inserted_total = 5
entries_erased_total = 0 <-- 5 entries were erased
Expected: the second block should report the 7 nodes released and the 5 entries erased by eraseTree() (compare with commit 6eaa079/ShowMemoryTest.cpp, which verifies that exactly this
path frees everything).
Secondary defect in the same block
eraseTree() increments accum.nodes_released itself (2827) and then calls writeContext->release(n) for the same node (2845-2885), which increments the same counter again:
So the increment at 2827 is a double count that is currently invisible only because init()
discards the whole accum. Simply dropping the init() reset would make every node released by eraseTree() count twice; the manual increments at 2826/2827 should be removed (or accum should
be saved/restored around init()).
Impact
nodes_allocated_total - nodes_released_total and entries_inserted_total - entries_erased_total are exactly the panels in grafana/dashboard.json
(panels 10 and 11: rate(nodes_allocated_total) - rate(nodes_released_total), etc.), and ServerBench.cpp exports these metrics to Prometheus. For any workload that ever moves newestVersion more than two billion past the previous one — a completely valid operation per include/ConflictSet.h ("All versions are >= 0", addWrites only requires writeVersion >= newestVersion), and the path exercised by conflict_set_cxx_api_test.cpp and ShowMemoryTest.cpp — the "live node"/"net insertion" gauges show one-sided growth that never
comes back down, and the dashboard cannot be used to detect the memory-reclamation behaviour the
project claims to maintain (paper §"Reclaiming old entries").
Suggested fix
In addWrites(), either
move the accum bookkeeping out of init() (e.g. have init() accept/keep the running totals,
or snapshot writeContext.accum around the eraseTree()/init() pair and add it back), and
delete the manual entries_erased/nodes_released increments in eraseTree() so the counting
happens only in release()/erase().
A regression test could extend ShowMemoryTest.cpp: after the version jump in its second block,
assert nodes_released_total/entries_erased_total are non-zero (or that nodes_allocated_total == nodes_released_total plus the nodes that are still live).
## Summary
`eraseTree()` (the whole-tree teardown used by the version-window reset) counts the nodes it
releases and the entries it erases into `writeContext->accum`:
```c++
// ConflictSet.cpp:2822-2828
while (toFree.size() > 0) {
auto *n = toFree.back();
toFree.pop_back();
writeContext->accum.entries_erased += n->entryPresent; // 2826
++writeContext->accum.nodes_released; // 2827
...
```
but every one of those counts is **thrown away** before `addWrites()` reports the metrics,
because `addWrites()` calls `init()` immediately after `eraseTree()`, and `init()` destroys and
reconstructs the `WriteContext`, which zeroes `accum`:
```c++
// ConflictSet.cpp:5170-5173
if (oldestExtantVersion < writeVersion - kMaxCorrectVersionWindow) [[unlikely]] {
if (writeVersion - kNominalVersionWindow > newestVersionFullPrecision) {
eraseTree(rootParent->children[0], &writeContext); // 5172
init(writeVersion - kNominalVersionWindow); // 5173
// ConflictSet.cpp:5379-5388
void init(int64_t oldestVersion) {
...
writeContext.~WriteContext(); // 5387
new (&writeContext) WriteContext(); // 5388 <-- accum is now all zero
```
`WriteContext() { memset(&accum, 0, sizeof(accum)); }` (ConflictSet.cpp:878), so by the time
`addWrites()` reaches `nodes_released_total.add(writeContext.accum.nodes_released)` (5236) and
`entries_erased_total.add(writeContext.accum.entries_erased)` (5239) both are 0.
Result: the two counters documented as *"The total number of physical tree nodes released"* and
*"The total number of entries erased from the tree"* (ConflictSet.cpp:5529-5532) never report any
of the work done by the version-window reset — the one path that releases an entire tree in a
single call.
## Reproduction
```c++
#include <ConflictSet.h>
#include <cstdio>
#include <cstring>
using namespace weaselab;
static void dump(ConflictSet &cs, const char *label) {
ConflictSet::MetricsV1 *m;
int count;
cs.getMetricsV1(&m, &count);
printf("=== %s\n", label);
for (int i = 0; i < count; ++i)
if (strstr(m[i].name, "nodes") || strstr(m[i].name, "entries"))
printf(" %-26s = %g\n", m[i].name, m[i].getValue());
}
static void writePoints(ConflictSet &cs, int64_t &v) {
ConflictSet::WriteRange w;
for (const char *k : {"a", "b", "c", "d"}) {
w.begin = {(const uint8_t *)k, 1};
w.end = {nullptr, 0};
cs.addWrites(&w, 1, ++v);
}
}
int main() {
{ // The same 4 entries, erased by garbage collection -> counted correctly
ConflictSet cs{0};
int64_t v = 0;
writePoints(cs, v);
cs.setOldestVersion(v); // gc erases all of them
dump(cs, "erased by setOldestVersion/gc");
}
{ // The same 4 entries, erased by the version-window reset -> not counted
ConflictSet cs{0};
int64_t v = 0;
writePoints(cs, v);
ConflictSet::WriteRange w;
w.begin = {(const uint8_t *)"zzz", 3};
w.end = {nullptr, 0};
cs.addWrites(&w, 1, int64_t(1) << 40); // eraseTree() + init()
dump(cs, "erased by eraseTree/init (version jump)");
}
}
```
Observed (clang 22, aarch64, `-O1`, default options):
```
=== erased by setOldestVersion/gc
nodes_allocated_total = 9
nodes_released_total = 4
entries_inserted_total = 4
entries_erased_total = 4
=== erased by eraseTree/init (version jump)
nodes_allocated_total = 10
nodes_released_total = 0 <-- 7 nodes were released
entries_inserted_total = 5
entries_erased_total = 0 <-- 5 entries were erased
```
Expected: the second block should report the 7 nodes released and the 5 entries erased by
`eraseTree()` (compare with commit `6eaa079`/`ShowMemoryTest.cpp`, which verifies that exactly this
path frees everything).
## Secondary defect in the same block
`eraseTree()` increments `accum.nodes_released` itself (2827) *and then* calls
`writeContext->release(n)` for the same node (2845-2885), which increments the same counter again:
```c++
// ConflictSet.cpp:890-895
template <class T> void release(T *c) {
++accum.nodes_released; // 893
```
So the increment at 2827 is a double count that is currently invisible only because `init()`
discards the whole `accum`. Simply dropping the `init()` reset would make every node released by
`eraseTree()` count twice; the manual increments at 2826/2827 should be removed (or `accum` should
be saved/restored around `init()`).
## Impact
`nodes_allocated_total - nodes_released_total` and
`entries_inserted_total - entries_erased_total` are exactly the panels in `grafana/dashboard.json`
(panels 10 and 11: `rate(nodes_allocated_total) - rate(nodes_released_total)`, etc.), and
`ServerBench.cpp` exports these metrics to Prometheus. For any workload that ever moves
`newestVersion` more than two billion past the previous one — a completely valid operation per
`include/ConflictSet.h` ("All versions are >= 0", `addWrites` only requires
`writeVersion >= newestVersion`), and the path exercised by `conflict_set_cxx_api_test.cpp` and
`ShowMemoryTest.cpp` — the "live node"/"net insertion" gauges show one-sided growth that never
comes back down, and the dashboard cannot be used to detect the memory-reclamation behaviour the
project claims to maintain (paper §"Reclaiming old entries").
## Suggested fix
In `addWrites()`, either
* move the `accum` bookkeeping out of `init()` (e.g. have `init()` accept/keep the running totals,
or snapshot `writeContext.accum` around the `eraseTree()`/`init()` pair and add it back), and
* delete the manual `entries_erased`/`nodes_released` increments in `eraseTree()` so the counting
happens only in `release()`/`erase()`.
A regression test could extend `ShowMemoryTest.cpp`: after the version jump in its second block,
assert `nodes_released_total`/`entries_erased_total` are non-zero (or that
`nodes_allocated_total == nodes_released_total` plus the nodes that are still live).
weaselbot
was assigned by andrew2026-11-10 19:09:58 +00:00
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.
Summary
eraseTree()(the whole-tree teardown used by the version-window reset) counts the nodes itreleases and the entries it erases into
writeContext->accum:but every one of those counts is thrown away before
addWrites()reports the metrics,because
addWrites()callsinit()immediately aftereraseTree(), andinit()destroys andreconstructs the
WriteContext, which zeroesaccum:WriteContext() { memset(&accum, 0, sizeof(accum)); }(ConflictSet.cpp:878), so by the timeaddWrites()reachesnodes_released_total.add(writeContext.accum.nodes_released)(5236) andentries_erased_total.add(writeContext.accum.entries_erased)(5239) both are 0.Result: the two counters documented as "The total number of physical tree nodes released" and
"The total number of entries erased from the tree" (ConflictSet.cpp:5529-5532) never report any
of the work done by the version-window reset — the one path that releases an entire tree in a
single call.
Reproduction
Observed (clang 22, aarch64,
-O1, default options):Expected: the second block should report the 7 nodes released and the 5 entries erased by
eraseTree()(compare with commit6eaa079/ShowMemoryTest.cpp, which verifies that exactly thispath frees everything).
Secondary defect in the same block
eraseTree()incrementsaccum.nodes_releaseditself (2827) and then callswriteContext->release(n)for the same node (2845-2885), which increments the same counter again:So the increment at 2827 is a double count that is currently invisible only because
init()discards the whole
accum. Simply dropping theinit()reset would make every node released byeraseTree()count twice; the manual increments at 2826/2827 should be removed (oraccumshouldbe saved/restored around
init()).Impact
nodes_allocated_total - nodes_released_totalandentries_inserted_total - entries_erased_totalare exactly the panels ingrafana/dashboard.json(panels 10 and 11:
rate(nodes_allocated_total) - rate(nodes_released_total), etc.), andServerBench.cppexports these metrics to Prometheus. For any workload that ever movesnewestVersionmore than two billion past the previous one — a completely valid operation perinclude/ConflictSet.h("All versions are >= 0",addWritesonly requireswriteVersion >= newestVersion), and the path exercised byconflict_set_cxx_api_test.cppandShowMemoryTest.cpp— the "live node"/"net insertion" gauges show one-sided growth that nevercomes back down, and the dashboard cannot be used to detect the memory-reclamation behaviour the
project claims to maintain (paper §"Reclaiming old entries").
Suggested fix
In
addWrites(), eitheraccumbookkeeping out ofinit()(e.g. haveinit()accept/keep the running totals,or snapshot
writeContext.accumaround theeraseTree()/init()pair and add it back), andentries_erased/nodes_releasedincrements ineraseTree()so the countinghappens only in
release()/erase().A regression test could extend
ShowMemoryTest.cpp: after the version jump in its second block,assert
nodes_released_total/entries_erased_totalare non-zero (or thatnodes_allocated_total == nodes_released_totalplus the nodes that are still live).