perf(store): size the pack body cache to the working set - #460
Merged
Conversation
The cache held four packfile bodies. That is two problems: its real size was four times whatever a pack weighed, a figure nobody stated, and four is below the working set of an ordinary repository. Being below the working set is what made a miss matter, because a miss re-reads an entire packfile to return one small object. On a 50,000-file repository with two snapshots and six packs -- the shape the memory benchmark builds -- check issues 127,789 lookups and misses 0.39% of them against a four-pack cache. Those 495 misses re-read about 9 MB each, which is the 4.2 GB of transfer behind check allocating 3 GB in the benchmark. At six packs the same trace misses six times. The access order was never the problem. A byte budget makes the ceiling a stated constant instead of an emergent product, and gives a repository with smaller packs proportionally more of them, which a count could not express. check on that repository: allocation 4.80 GB to 550 MB, peak RSS 266 MB to 204 MB. Both improve, because the re-read transients dominated peak as well. No change at sizes whose working set already fit. The budget is deliberately larger than what it replaces. Going the other way was measured and is worse: at 32 MB the same check peaks at 251 MB against 167 MB, since the transfers a smaller cache forces cost more than the residency it saves. Part of #458.
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
packCacheis bounded by total bytes rather than by a fixed count of four bodiesHolding four bodies meant the cache's real size was four times whatever a pack
weighed — and four is below the working set of an ordinary repository, which is
what made a miss matter: a miss re-reads an entire packfile to return one small
object.
Part of #458.
Measurements
checkon a 50,000-file repository with two snapshots and 6 packs — the shapethe benchmark builds, and the one where its
checkallocation jumps to ~3 GB:main@051a07eBoth improve, because the re-read transients dominated peak as well as
cumulative allocation.
No regression where the working set already fit:
mainWhy this and not an ordering fix
The issue as I filed it guessed at a scattered access pattern. Tracing showed the
opposite:
checkissues 127,789 lookups across 6 packs and misses 0.39% ofthem against a four-pack cache. There is nothing like restore's 55% before #455.
Those 495 misses cost 4.2 GB because each one re-reads ~9 MB. At six packs the
same trace misses six times. The order was already good; the cache was smaller
than the order needs.
The budget is deliberately larger than what it replaces. Shrinking it was
measured and is worse — at 32 MB the same
checkpeaks at 251 MB against 167 MB,because the transfers a smaller cache forces cost more than the residency it
saves (#440 comment).
Note on the regression test
TestPackStore_DoesNotRereadPacksWhenTheWorkingSetFitsasserts on bytestransferred, not on whole-pack count. My first version asserted on the count
and passed even with a one-pack budget: since #452, a too-small cache does not
do more whole-pack transfers, it does more ranged reads, so the count barely
moves while the traffic triples.
The fixture also writes more than
packPromoteAfterobjects per pack. With fewer,a pack is never cached at all — every read stays a ranged read — and the test
measures promotion behaviour rather than the cache.
Both directions verified: it passes at the shipped budget and fails at a one-pack
budget (3.1 MB read against 806 KB of packs).
Verification
Both pass; lint reports 0 issues.
checkverifies an identical 10,993 objectswith 0 errors before and after.
One figure I cannot fully account for
At 5,000 files, allocation drops from 115 MB to 3.5 MB while peak RSS is
unchanged and the object count verified is identical. The direction is expected —
that repository's packs now stay cached — but the magnitude is larger than the
pack sizes explain, so treat it as unexplained rather than as a headline. The
50,000-file figures above are the ones I would rely on.