perf(store): serialize the pack catalog once when compacting - #449
Conversation
CompactCatalog marshalled the whole catalog twice and copied it once:
writeShard marshalled it to derive the key it wrote to, and mustMarshal
then marshalled the identical map again purely to recompute that key.
On a 50k-file repository each marshal is a ~19 MB byte slice, and both
were live alongside a duplicate of the catalog map.
Split sealing from writing. sealShard renders a shard and returns the
key it belongs under, doing no I/O, so compaction can seal the catalog
in place under the read lock it already holds and write afterwards --
no copy, one marshal. writeShard is sealShard plus a Put, and returns
the key so no caller has to derive it a second time.
That also removes a way for the two derivations to disagree. mustMarshal
returned nil on error, so a failed marshal named the hash of no bytes:
compaction would record a consolidated shard that was never written and
delete the shards it was meant to replace. The same key appeared for an
emptied catalog, where writeShard wrote nothing but the caller still
recorded the hash of "{}" in mergedIndex -- an absorbed object that does
not exist. An empty catalog now records no consolidated shard, and still
removes the inputs, which is the deletion the compaction exists to make
durable.
Closes #438
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
sealShard(render a shard, return its key and bytes, no I/O) fromwriteShard(seal plusPut, returning the key it wrote)CompactCatalogseals the catalog in place under the read lock it already holds, so it neither copies the catalog nor marshals it twice{}as an absorbed object that was never writtenCompactCatalogmarshalled the catalog twice and copied it once:writeShardmarshalled it to derive the key it wrote to, then
mustMarshalmarshalled theidentical map again solely to recompute that key. On a 50k-file repository each
marshal is a ~19 MB byte slice, both live alongside the duplicate map.
Closes #438
The correctness half
mustMarshalreturnednilon error, so a failed marshal namedcore.ComputeHash(nil)— the hash of no bytes. Compaction would then record aconsolidated shard that was never written and delete the shards it was meant to
replace.
The same key showed up on a path that needs no marshal failure at all. When every
entry has been deleted,
writeShardreturns early without writing, but the callerstill computed
shardPrefix + ComputeHash(mustMarshal(merged))and stored it inmergedIndex. Verified againstmainatee61760:That is
sha256("{}").TestCompactCatalog_EmptiedCatalogRemovesItsShardsfailson
mainand passes here.Note on the lock
Sealing now happens under
s.mu.RLockrather than after copying the catalog out.Sealing does no I/O — the
Putstill happens after the unlock — so the lock isheld for a marshal rather than a round trip, and it is a read lock, so concurrent
readers are unaffected. A repository large enough for compaction to matter is
exactly the one whose catalog is expensive to duplicate.
Verification
Both pass; lint reports 0 issues.