perf(engine): stop retaining filemetas nothing reads - #433
Merged
Conversation
#429 proposed bounding the filemeta loader with an LRU, the way NodeStore bounds its node cache. Benchmarking that first showed it would be a regression, and pointed at two better targets. The LRU does not work here. A filemeta traversal sweeps each ref once per snapshot, uniformly, so an LRU evicts precisely the entry the next sweep asks for — the hit rate does not degrade, it collapses. Over eight sweeps of a 10k-file tree a 4096-entry cache turns 10,000 store reads into 80,000. NodeStore is bounded successfully because a HAMT descent re-touches the root and upper levels on every lookup; there is no equivalent locality here. Any fixed bound has this cliff, so the cache stays unbounded and the memory came out of the traversals instead. prune memoized filemetas it could never re-read: markFileMeta returns early for a ref already in its reachable set, so a load happens at most once per run however many snapshots share the tree. Measured over four snapshots of a 2000-file tree, the cache ended with 2000 entries and zero hits. It now reads through — 54.6 MB to 6.7 MB retained at 100k files. diff's parent lookup retained every entry, but is read at exactly one place, byID[parentID], and a parent is a folder. Keeping only folders takes it from 210,004 entries to 10,004 on a 100k-file tree, and peak heap from 275.5 MB to 217.6 MB. An entry naming a non-folder parent is unaffected: collectMetaPaths already ends a chain at the deepest ancestor it resolves. Neither change costs a store read, which is what separates them from bounding the cache. Adds the benchmarks that produced these numbers, tests pinning both choices (each regression is silent — a cache that stops being consulted still works, and one that starts retaining everything still returns the right answer), and corrects docs/caching.md, which claimed no cache was redundant. prune's was. Closes #429
collectMetadata and byID described the map before it was filtered — it now holds only folders, so collectFolders and folderByID say so at every use site rather than leaving the reader to infer it from the loop body. This also separates it from RestoreManager.collectMetadata, which shares the old name but not the semantics: restore writes every entry, so it genuinely needs them all. A comment marks the distinction so the two are not re-converged later.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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
#429 proposed bounding the filemeta loader with an LRU, the way
NodeStorebounds its node cache. I benchmarked that first — it would have been a regression. The measurement pointed at two better targets, and this PR does those instead.Why the LRU doesn't work
A filemeta traversal sweeps each ref exactly once per snapshot, uniformly. Under a cyclic sweep an LRU evicts precisely the entry the next sweep is about to ask for, so the hit rate doesn't degrade — it collapses. From
BenchmarkMetaLoaderDiffPattern, eight sweeps:At 10k files a 4096-entry cache turns 10,000 store reads into 80,000 — 8× the API calls on a remote backend.
NodeStoreis bounded successfully because a HAMT descent re-touches the root and upper levels on every lookup; a filemeta sweep has no equivalent locality. Any fixed bound has this cliff, it just moves. So the cache stays unbounded and the memory came out of the traversals instead.prunememoized filemetas it could never re-readmarkFileMetareturns early for a ref already in itsreachableset, so a load happens at most once per run however many snapshots share the tree. Measured over four snapshots of a 2000-file tree: the cache ended with 2000 entries and zero hits. It now reads through.diffretained every entry in a folder-only lookupcollectMetadata's map is read at exactly one place —byID[parentID]intoFileChange— and a parent is a folder. Retaining files there held acore.FileMetaeach for entries nothing read.An entry naming a non-folder parent is unaffected:
collectMetaPathsalready ends a chain at the deepest ancestor it can resolve, which is its documented behaviour for any unresolvable parent.Neither change costs a single store read — that is what separates them from bounding the cache.
Closes #429
What's added
metaloader_bench_test.go— the two benchmarks that produced these numbers: retained bytes vs file count, and hit rate under the diff access pattern.metaloader_memory_test.go— tests pinning both choices. Both regressions are silent, which is why they need pinning: a cache that stops being consulted still works, and one that starts retaining everything still returns the right answer.TestDiffRetentionGrowsWithFoldersNotFilesasserts on entry counts rather than heap, which is too noisy to assert on.TestDiffReportsFullPathsAfterFilteringproves the filter doesn't shorten a nested path — the point being that the dropped entries were unread.Reviewer notes
docs/caching.mdis corrected. It claimed no cache was redundant.prune's was, and I only found it by counting hits rather than reading the code — which is why the benchmarks now exist. The doc says so explicitly rather than quietly editing the claim.diff's remaining peak is dominated by the loader cache, which isO(tree)by design. Reducing it further means not walking both roots in full — an algorithm change, not a cache change, and out of scope here.Verification
env GOCACHE=/tmp/cloudstic-gocache go test -race -count=1 ./...passes (full module)env GOCACHE=/tmp/cloudstic-gocache GOLANGCI_LINT_CACHE=/tmp/cloudstic-golangci-lint golangci-lint run ./...— 0 issuesnpx markdownlint-cli2 '**/*.md'— 0 issuesDocker-backed e2e (MinIO, SFTP) skipped locally for want of
/var/run/docker.sock.