maedhroz commented on PR #4900:
URL: https://github.com/apache/cassandra/pull/4900#issuecomment-4791782382
Pasting the bits surfaced from a quick CC skill-assisted review here, and
then I'll do a more careful manual review:
```
Finding 1: if (allocated > 0) silently swallows net releases — contradicts
patch's stated net-delta semantics
- Location: src/java/org/apache/cassandra/utils/btree/BTree.java:3713-3714
- Confidence: High (latent active bug; certain consistency hazard)
- Flagged by: Logic, Boundary, Resources, Completeness, Symmetry (5
specialists)
- What's wrong: After the patch, Updater.allocated is documented and used
as a true net delta (addAllocated adds for newly retained nodes, subtracts for
replaced sources, root release included). But the report site still gates on
allocated > 0, dropping any zero or negative result. This collides on two axes:
(a) the sentinel -1
overloads "tracking disabled" with "net release" — once allocated goes
below zero it stays negative and future addAllocated calls become no-ops
because the helper guards on allocated >= 0; (b) the parallel report sites at
BTree.java:382, 397 are unconditional/signed, so this is the only asymmetric
guard.
- Why current code doesn't trigger it: BTree.update's reconciler is 1-to-1
(no key removal), so per-step addAllocated arguments are non-negative across
all paths the symmetry/logic agents traced (drain, redistributeAndDrain,
redistributeOverflowAndDrain, propagateOverflow). The patch's
BTreeUpdateHeapAccountingTest does not
exercise a net-zero-or-negative outcome — the closest case (root reuse on
identical-shape merge) is not in the scenario set.
- Suggested fix: replace the report condition with if (allocated != 0) and
decouple "tracking disabled" from the value of allocated (e.g. a boolean
accountingEnabled set once at the start of update). Add a scenario to
BTreeUpdateHeapAccountingTest where every key in insert already exists with
identical content (root reuse →
allocated == 0).
Finding 2: UpdateFunction.onAllocatedOnHeap javadoc still describes the
old semantics
- Location:
src/java/org/apache/cassandra/utils/btree/UpdateFunction.java:46-49
- Confidence: High
- Flagged by: Completeness
- What's wrong: Javadoc reads @param heapSize extra heap space allocated
(over previous tree). After this patch the parameter is a signed net delta —
the new BTreeRow.merge and ColumnData.removeShadowed call sites can pass
negative values today. Future implementers reading the contract may add a
non-negative assert and silently
break accounting.
- Suggested fix: state explicitly that the value is a signed delta and may
be negative when more heap is released than allocated.
Finding 3: AbstractFastBuilder.reset() does not clear the leaf-level
sourceNode; the new root setSourceNode(update) widens the leak window
- Location: src/java/org/apache/cassandra/utils/btree/BTree.java:3511-3532
(reset) and :3706 (the new root assignment)
- Confidence: Medium
- Flagged by: Concurrency, Symmetry
- What's wrong: reset() walks branch = leaf().parent clearing each
parent's sourceNode, but never clears the leaf builder's own sourceNode.
Pre-patch this was an idle latent issue because LeafBuilder.drain() always
cleared it on the success path. Post-patch, Updater.update() now eagerly sets
builder.setSourceNode(update) before
updateRecursive runs — so any exception thrown between line 3706 and the
next drain() returns the Updater to the FastThreadLocal pool with a dangling
reference to the previous update tree's root array. The dangling ref is
eventually overwritten on the next update() reaching the leaf level, but until
then the pooled Updater
pins memory.
- Suggested fix: add clearSourceNode() (the leaf-level call on this) at
the top or bottom of AbstractFastBuilder.reset().
Finding 4: LeafBuilder.propagateOverflow not migrated to
addAllocated(shallowHeapOf(savedBuffer))
- Location: src/java/org/apache/cassandra/utils/btree/BTree.java:2896-2905
- Confidence: Medium (consistency, not correctness today)
- Flagged by: Boundary, Concurrency, Resources, Symmetry (4 specialists)
- What's wrong: Every other allocation/release point in
LeafBuilder/BranchBuilder was rewritten through addAllocated(...) plus
shallowHeapOf(...)/sizeOnHeapOfLeaf(...); this one site still uses the
inline-guarded allocated += ObjectSizes.sizeOfReferenceArray(MAX_KEYS).
Numerically equivalent today (savedBuffer is new
Object[MAX_KEYS]), but the inconsistency is exactly the kind of asymmetric
edit that produced four of the five bugs the patch is fixing.
- Suggested fix: replace with
leaf.addAllocated(shallowHeapOf(savedBuffer));.
Finding 5: recordDeletion.onAllocatedOnHeap(...) in removeShadowed is
computed and discarded on the noOp path
- Location: src/java/org/apache/cassandra/db/rows/ColumnData.java:230-232
(with noOp defined at :67-89)
- Confidence: Low (efficiency, not correctness)
- Flagged by: Completeness
- What's wrong: When recordDeletion == ColumnData.noOp (the update side /
Reconciler.merge path), the new line still computes EMPTY_SIZE +
sizeOnHeapOf(existingTree) and EMPTY_SIZE + sizeOnHeapOf(cells) (each
potentially walking a multi-node tree) only to pass the result to a no-op.
Trivial fix: gate on recordDeletion != noOp.
Finding 6: removeShadowed does not subtract complexDeletion.dataSize()
when a complex column is dropped
- Location: src/java/org/apache/cassandra/db/rows/ColumnData.java:200-233
- Confidence: Low (out of patch scope; small dataSize drift, not heap)
- Flagged by: Resources
- What's wrong: BTreePartitionUpdater.insert(ColumnData) originally added
insert.dataSize() (which for ComplexColumnData includes
complexDeletion.dataSize() == 12). The patch's release path subtracts each
cell's dataSize via recordDeletion.delete and the wrapper+tree heap via
onAllocatedOnHeap, but never subtracts the
complexDeletion.dataSize(). Heap accounting (the patch's stated target) is
fine; dataSize accumulates 12 stranded bytes per dropped complex column with a
non-LIVE complexDeletion. Adjacent to the patch, would be a one-line fix.
Finding 7: Latent paths that don't release sourceNode if reached
- Location: LeafBuilder.drain() count==0 early return at
BTree.java:2951-2956; BranchBuilder.drain() count==0 / right-child collapse at
:3261-3270
- Confidence: Low (currently unreachable from the patched call paths)
- Flagged by: Resources, Symmetry
- What's wrong: Both early-return branches clearSourceNode() without first
releasing the source's shallow heap through addAllocated. BTree.update's
reconciler doesn't drop keys, so neither branch is reachable from the Updater
today. Worth either an assertion or a defensive
addAllocated(-shallowHeapOf(sourceNode)) so the new
"every replaced source releases its shallow heap" invariant holds globally.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
