perf(store): name pending catalog entries instead of copying them - #456
Conversation
prepareFlushLocked wrote every bundled object into two maps: catalog, and pendingShard as a second full copy so a flush knew what to write. By the time a backup flushes, pendingShard holds a whole run's worth of entries. It was the largest single allocation in a backup -- 50.2 MB flat, 41% of live heap at 50k files. pendingShard becomes pendingKeys, a set. The values are already in catalog and the key strings are shared with it, so an entry costs a header and a map slot rather than a duplicate entry. Flush renders the shard directly from the catalog through sealShardFor, rather than materialising the pending entries into a map to marshal -- which would have reinstated the duplicate at the moment it matters most. Entries are sorted so the bytes, and the content-addressed shard key, are identical to marshalling an equivalent map; a test pins that against sealShard. The set keeps its second job from #444: it records which catalog entries are authoritative, so a failed catalog load can drop streamed remote entries and keep local and footer-recovered ones. Nothing but a local write or a footer rebuild puts a key there, and mergePackIndex is first-writer-wins, so a pending key's value cannot have come from the failed load. Backup at 50k files: live heap 141 MB to 101.6 MB, peak RSS 360 MB to 336 MB. prepareFlushLocked leaves the profile's top entries entirely. Closes #437
|
Warning Review limit reached
Next review available in: 3 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 (7)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
pendingShard map[string]PackEntrybecomespendingKeys map[string]struct{}— the values are already incatalogand the key strings are shared with itFlushrenders the shard straight from the catalog viasealShardFor, rather than materialising the pending entries into a map to marshalBy the time a backup flushes,
pendingShardheld a whole run's worth of entries.It was the largest single allocation in a backup: 50.2 MB flat, 41% of live
heap at 50k files.
Closes #437
Measurements
Backup, 50,000 files:
main@f4deb56prepareFlushLockedleaves the profile's top entries entirely — it was thenumber one line before.
Why this rather than #440
This started as #440. Profiling
mainfirst put the growth somewhere else thanRFC 0023 assumed, so the work followed the measurement:
prepareFlushLocked50.2 MB — this PRpackCache36 MB, catalog ~20 MB,verifiedmap 7 MBThe pack catalog — #440's target — is roughly 20 MB of
check's 74.5 MB, behindthe 36 MB body cache. Bounding it is still worth doing, but it is not where the
largest growth was, and #437 was blocked on #440 only because of a coupling this
PR resolves.
This reduces the linear coefficient; it does not remove the growth. The
catalog, the
verifiedset and backup's cachedmetaLoaderare all stillO(repository). I have written up what remains on #440.
On the authoritative-entry role
The set keeps the second job
pendingShardacquired in #444: recording whichcatalog entries are authoritative, so a failed catalog load can drop streamed
remote entries and keep locally written and footer-recovered ones. That still
holds, because nothing but a local write or a footer rebuild puts a key there,
and
mergePackIndexis first-writer-wins — so a pending key's value in thecatalog cannot have come from the failed load.
Verification
Both pass; lint reports 0 issues. New tests cover the byte-identical rendering
against
sealShard, a pending key whose catalog entry has been removed, and anempty pending set. The existing pack durability and upload-failure tests cover
the restore-on-failure paths, which now restore keys rather than entries.