perf(engine): hold prune's reachable and check's verified sets compactly - #499
Merged
Conversation
prune's mark phase and check's walk each hold one map entry per object in the repository. Both were map[string]bool keyed by "<prefix>/<64 hex>" — 73 bytes of text plus, as a map key, an interior pointer the garbage collector must trace, all to carry 32 bytes of hash. internal/storelayer already solved this for the pack catalog, and internal/storelayer/keycache.go a second time for its digest sets. Rather than add a third hand-rolled copy of the same bit packing, this factors it into internal/objkey: a Key (namespace byte plus decoded SHA-256), the canonical-lowercase hex decoder both existing copies needed, and the Set that prune and check now use. packCatalog and KeyCacheStore are rewired onto it, so there is one encoding rather than three drifting ones. Measured at 67 B/entry against 132 for map[string]bool over 200,000 keys (BenchmarkSetBytesPerEntry), so ~65 bytes and one traced pointer per live object, on the two structures whose size is the repository's rather than the work in front of them. The encoding is total, which for prune is correctness rather than polish: docs/compatibility.md forbids a garbage collector from reading "cannot represent" as "not referenced", and a key missing from the reachable set is an object the sweep deletes. A key that does not fit the compact shape is kept verbatim in a string-keyed fallback, and TestPruneManager_KeepsObjectsWhoseKeysDoNotFitTheCompactForm walks a repository whose chunk refs take those shapes and requires them to survive. Only canonical lowercase hex decodes, so encoding is injective. That fixes a latent bug in packCatalog, which decoded with encoding/hex: uppercase hex decoded there and re-encoded lowercase, so such a key round-tripped through Each under a different name than it went in with.
rmanibus
added
area/core
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds compact object-key encoding and fallback storage. It applies ChangesObject-key foundation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/objkey/objkey.go (1)
40-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the namespace encoding table immutable.
Line 40 exposes the namespace table as a mutable slice. A caller can reorder, truncate, or modify it after a
Keyis stored.Encode,Key.String, andSetmembership can then use different encodings. Prune can fail to recognize a reachable key.Make the table private. Expose a copy or an iterator for tests and package consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/objkey/objkey.go` at line 40, Make the namespace table backing Namespaces private and prevent callers from mutating the encoding order used by Encode, Key.String, and Set. Update internal references to use the private table, and provide a copy-returning accessor or iterator for tests and package consumers that need to inspect the namespaces without altering them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/objkey/objkey.go`:
- Line 40: Make the namespace table backing Namespaces private and prevent
callers from mutating the encoding order used by Encode, Key.String, and Set.
Update internal references to use the private table, and provide a
copy-returning accessor or iterator for tests and package consumers that need to
inspect the namespaces without altering them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81ad65a4-3581-410a-9e38-1deeeae85914
📒 Files selected for processing (12)
AGENTS.mdinternal/engine/check.gointernal/engine/prune.gointernal/engine/prune_guard_test.gointernal/objkey/objkey.gointernal/objkey/objkey_test.gointernal/objkey/set.gointernal/objkey/set_bench_test.gointernal/storelayer/keycache.gointernal/storelayer/keycache_test.gointernal/storelayer/packcatalog.gointernal/storelayer/packcatalog_test.go
The table's index is the namespace byte, so reordering it desynchronises Encode from Key.String and from every Key already in a Set — a reachable key prune would then fail to recognise. The doc comment asked callers to treat it as append-only; an exported slice left that advisory, and mutable at run time between one Encode and the next. Unexport it and return a copy from Namespaces(), so the invariant cannot be broken from outside the package at all. Raised by CodeRabbit on #499.
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
1 participant
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
internal/objkey: the compact in-memory form of an object key (Key— anamespace byte plus a decoded SHA-256), the canonical-lowercase hex decoder,
and the
Setbuilt on them. A key is<namespace>/<64 hex>— 73 bytes of textplus, as a map key, an interior pointer the garbage collector traces, all to
carry 32 bytes of hash.
in front of them: prune's
reachableset (internal/engine/prune.go) andcheck's
verifiedset (internal/engine/check.go), both previouslymap[string]bool.packCatalogandKeyCacheStoreonto the same package. This bitpacking was already written twice in
internal/storelayer; prune and checkwould have been a third and a fourth copy, which is how such a scheme drifts.
docs/compatibility.mdforbids a garbage collector from reading "cannotrepresent" as "not referenced", and a key missing from
reachableis an objectthe sweep deletes. A key that does not fit the compact shape is kept verbatim in
a string-keyed fallback.
Encodeis a pure function of the key, soAddandHasalways consult the same map; and it is injective, so a compact entry canonly ever be reported for the key that added it.
packCatalogdecoded withencoding/hex,which accepts uppercase. An uppercase key therefore encoded, and re-encoded
lowercase — so it round-tripped through
Eachunder a different name than itwent in with, and prune's sweep would have been handed the name of an object
that is not there. Only canonical lowercase decodes now.
Related issues
None.
Repository compatibility
No repository format change. A
Keyis in-process only — it is never written toa store, and every caller (
Set,packCatalog.Each,packCatalog.Keys) handsback full key strings.
The change nonetheless touches a garbage collector, so the compatibility argument
is the point of the PR rather than a formality.
docs/compatibility.md: "Neverread 'cannot decode' as 'empty'" and "never let a destructive operation proceed
on incomplete information". A representation that silently dropped a key it
could not encode would violate both from the inside — prune would mark fewer
objects than it reached, and delete the difference, with nothing to signal it.
So
objkey.Setis total over strings rather than over well-shaped keys, and thepackCatalogfallback that already existed is preserved with the same intent.This is covered by
TestPruneManager_KeepsObjectsWhoseKeysDoNotFitTheCompactForm,which walks a real repository whose chunk refs are short, legacy-named, and
uppercase-hex, and requires all of them to survive a prune that does delete real
garbage in the same run; by
TestSet_KeepsUnshapedKeysandTestEncode_IsInjectiveAcrossHexCase; and byTestPackCatalog_PreservesNonCanonicalHexExactly. The uppercase-hex fix strictlynarrows what encodes, so keys that previously round-tripped compactly still do.
Verification
Both pass; lint reports 0 issues. The prune guards
(
TestPruneManager_AbortsWhenSnapshotsVanishButObjectsRemain,TestPruneManager_AbortsWhenSweepListingFails) are unaffected.What an entry costs
BenchmarkSetBytesPerEntry/_Map, 200,000 keys, heap measured across the fillrather than read off
B/op(Apple M3 Max, go1.25):map[string]boolobjkey.SetBenchmarkSetHasis 0 allocs/op, andBenchmarkKeyCacheExistsstays at 0allocs/op after the decoder moved packages.
What
bench.shcan see: nothing, and that is the honest answerSAMPLES=1 SIZES="5000 20000 50000" BACKENDS=local scripts/benchmark/bench.shSAMPLES=1deliberately:bench.shregenerates the tree once per size and thechurn steps mutate it in place, so only sample 1 is comparable between runs. Two
independent repetitions were run per binary, which gives a same-binary spread to
judge the difference against.
Every delta is inside the spread of the same binary against itself. The last two
rows are the control:
restoreandbackuptouch neither set and cannot havechanged, yet they moved as much as or more than the operations that did. Reading
any of the first four rows as a result would be reading noise.
alloc_mbalso cannot show this change, and may show it inverted. The keystrings are allocated either way — they arrive from a store listing or a JSON
decode — so what changes is whether the set retains them; meanwhile the compact
map's buckets are wider (33-byte keys against 16-byte string headers), which adds
allocation volume. Residency down, allocation flat-to-slightly-up is the expected
shape, and
alloc_mbonly sees the second half of it.The structural argument
The benchmark's largest cell is too small for 65 B/entry to clear the noise
floor. Measured with
prune -dry-runon a 50,000-filesourcetree: 109,999objects. 65.1 B × 110,000 ≈ 7 MB, against a prune peak RSS of ~380 MB — under
2%, where the same-binary spread at that scale is 37 MB.
What matters is that this is the term that grows with the repository while the
rest of prune's and check's footprint does not. At 1M objects it is 65 MB; at
10M, 650 MB — the difference between an operation that opens a large repository
and one that does not. Peak RSS answers "does this grow with the repository",
and the per-entry number is the only place that question is visible at a scale
CI can run.
Documentation
AGENTS.mdgains aninternal/objkey/entry in the package layout, stating whatit is, why it is not in
internal/core(aKeyis never written down), and thatthe encoding is total. No exported API changed, so no update to the Cloudstic
docs repository is needed.
Summary by CodeRabbit