fix(hamt): re-merge under-full subtrees so tree shape is canonical - #423
Merged
Conversation
A HAMT's shape should be a function of its contents, not of the history that produced them. Insertion already was: buildNode is a pure function of the entry set, and a leaf splits at exactly the size buildNode stops at. Deletion was not. It dropped empty slots and promoted a lone remaining leaf, but never re-merged several under-full leaves, so a subtree that split under load and later shrank stayed split. Txn.delete now restores buildNode's own invariant as the recursion unwinds: an internal node exists only where more than maxLeafSize entries live beneath it. Applying it on the way up lets a collapse cascade without a second pass, and subsumes the single-leaf promotion except for a leaf that outgrew maxLeafSize, which can only happen at maxDepth where a leaf may not split. The check is budgeted rather than exhaustive. subtreeEntries abandons a subtree as soon as it has seen more entries than a leaf could hold, so a large subtree costs a couple of node loads instead of a traversal: one delete in a 4000-entry tree reads 16 nodes, against 532 without the budget. A test pins that, and fails if the budget is removed. Measured on histories resembling a repository rather than the synthetic shrink that surfaced this. Steady churn and wholesale directory removal were already canonical -- affinity routing means a deleted directory empties whole subtrees, which the existing rules handle -- so the damage came from scattered deletion without replacement, where a tree of 364 files carried 381 nodes against 32 for the same content built directly. That is now 32, and the run wrote 4268 nodes instead of 6907. There is no correctness change and no format version bump. Only which shapes get written changes, not how a shape is encoded, and a leaf mid-tree is what a bulk build already produces. Verified against v1.18.0 in both directions rather than reasoned about: it lists, ls, checks with -read-data, restores, diffs and backs up on top of a collapsed tree, and this build reads a tree an older one left non-canonical. A no-change backup still produces the same root, so -ignore-empty-snapshot is unaffected. The four pinned roots in TestRootHashGolden are unchanged, which is the evidence that the encoding is untouched; the golden gains a scenario, because the one named for collapse left 60 entries and so never reached the new rule.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Txn.deletenow re-merges a subtree back into a leaf once it holdsmaxLeafSizeentries or fewer, restoring the invariantbuildNodealready maintains on insertion: an internal node exists only where more than a leaf's worth of entries live beneath it.subtreeEntriesabandons a subtree as soon as it has seen more entries than a leaf could hold.TestRootHashIsDeletionHistoryIndependentas the deletion counterpart to the existing order-independence test, plus a traversal-budget test and a regression pin for whole-directory deletion.docs/spec.md.Closes #421.
Measurement first
The issue asked for a realistic measurement before committing to a change in a core data structure. That measurement changed the picture, and is worth reading before the diff:
The two cases I would have guessed were worst — steady churn and wholesale directory removal — were already exactly canonical. Affinity routing gives a directory a shared routing prefix, so deleting one empties whole subtrees, which the existing empty-slot and single-leaf rules already handle.
The damage comes from scattered deletion without replacement, and there it is worse than the issue estimated: 11.9× rather than the 5× the synthetic workload suggested. That run also wrote 6907 nodes where the fixed version writes 4268, so this reduces write amplification during the history, not only the final node count.
The two already-canonical workloads produce byte-identical node counts after the change (712 and 546), so nothing regressed to buy this.
Why not "sort the children"
Worth stating since it was the first instinct: children are already canonical, indexed by hash bits through the node bitmap, and insertion order already does not matter —
TestRootHashIsOrderIndependenthas pinned that all along. Deletion was the only asymmetry.Cost
subtreeEntriesstops once it has seenmaxLeafSize + 1entries, so a subtree too large to collapse is rejected after a handful of node loads. One delete in a 4000-entry tree reads 16 nodes; without the budget the same delete reads 532.TestCollapseDoesNotTraverseLargeSubtreespins this and fails if the budget is removed — verified by removing it.Compatibility
No format version bump, and no encoding change. What changes is which shapes get written, not how a shape is encoded. A leaf appearing mid-tree is already what a bulk build produces for any subtree small enough to fit in one.
Verified against a
v1.18.0binary in both directions rather than reasoned about:list,ls,check -read-data,restore,diff, andbackupon top all work against a repository whose tree collapsed to a single leaf.check -read-datareports it healthy.-ignore-empty-snapshotis unaffected. A no-change backup performs no deletions, so no collapse fires and the root is unchanged; the snapshot count stayed at 2.A non-canonical tree written by an older build stays valid and becomes canonical the next time a deletion passes through it, which is the opportunistic-upgrade behaviour
docs/compatibility.mdasks for.On the golden file:
roothash_test.gowarns that a changed root is a format change, not something to regenerate away. All four pinned roots are unchanged — that is the evidence the encoding is untouched. The golden gains one line because I added a scenario: the existing one named for collapse-on-delete leaves 60 entries, abovemaxLeafSize, so it never reached the new rule and the shape of a shrinking tree would have stayed unpinned.Verification
env GOCACHE=/tmp/cloudstic-gocache go test -count=1 ./...env GOCACHE=/tmp/cloudstic-gocache go test -race -count=1 ./internal/hamt ./internal/engine ./env GOCACHE=/tmp/cloudstic-gocache GOLANGCI_LINT_CACHE=/tmp/cloudstic-golangci-lint golangci-lint run ./...npx markdownlint-cli2 docs/spec.md