From b8fdc9dc5aba055be89957183860496f9de3a219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hermann?= Date: Tue, 4 Aug 2026 09:34:40 +0200 Subject: [PATCH 1/2] perf(engine): stop retaining filemetas nothing reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- docs/caching.md | 100 ++++++++++++-- internal/engine/diff.go | 14 +- internal/engine/metaloader_bench_test.go | 158 ++++++++++++++++++++++ internal/engine/metaloader_memory_test.go | 135 ++++++++++++++++++ internal/engine/prune.go | 5 +- 5 files changed, 396 insertions(+), 16 deletions(-) create mode 100644 internal/engine/metaloader_bench_test.go create mode 100644 internal/engine/metaloader_memory_test.go diff --git a/docs/caching.md b/docs/caching.md index bc705f6..cdb64db 100644 --- a/docs/caching.md +++ b/docs/caching.md @@ -2,8 +2,15 @@ Cloudstic caches in several places, at different layers, with different lifetimes. This document is the inventory: what each cache holds, how long it -lives, what bounds it, and — the question that keeps coming up — why none of -them is redundant with any other. +lives, what bounds it, and — the question that keeps coming up — which of them +overlap. + +The first version of this document claimed none was redundant. That was wrong +about one: `prune` memoized filemetas behind a `reachable` set that already +guaranteed one load per ref, so its cache could never return a hit. It has been +removed. The lesson is in how it was found — by counting hits rather than +reading the code, which is why the benchmarks in +`internal/engine/metaloader_bench_test.go` now exist. None of these caches is persistent. Every one lives in process memory and dies with the `Client` or the operation that created it. Nothing here is part of the @@ -18,7 +25,7 @@ event (`docs/compatibility.md`). | `PackStore.catalog` / `packKeys` | `internal/storelayer/pack.go` | object key → pack ref + offset | `Client` | one entry per packed object | | `PackStore.packCache` | `internal/storelayer/pack.go` | pack ref → raw packfile bytes | `Client` | LRU, 4 packs (~32 MB at 8 MB/pack) | | `NodeStore.cache` | `internal/hamt/nodestore.go` | node ref → decoded `*node` | `hamt.Tree` | LRU, 4096 nodes | -| `metaLoader.cache` | `internal/engine/metaloader.go` | filemeta ref → decoded `core.FileMeta` | manager | unbounded; opt-in per constructor | +| `metaLoader.cache` | `internal/engine/metaloader.go` | filemeta ref → decoded `core.FileMeta` | manager | unbounded; enabled only for `backup` and `diff` | | `findScanner.evaluated` | `internal/engine/find_scan.go` | 16-byte ref digest → match verdict | one `find` run | one small entry per distinct filemeta ref | | `Resolver.cache` | `pkg/secretref/secretref.go` | `scheme://path` → secret | `Resolver` | only `keychain`, `wincred`, `secret-service` | | `Client.repoIDCache`, `openCfg` | `client.go` | — → repository marker fields | `Client` | one value each | @@ -53,9 +60,11 @@ would mean paying for the work before discovering it was unnecessary. The engine's own caches sit above the chain entirely, holding decoded objects: `NodeStore` for `node/`, `metaLoader` for `filemeta/`. -## Why none of them is redundant +## Which ones overlap -The pairs that look like they overlap, and why each is a distinct job. +The pairs that look like they duplicate each other, and why each surviving one +is a distinct job. (The one that genuinely was redundant, `prune`'s, is gone — +see the note at the top.) ### `KeyCacheStore` vs `PackStore`'s catalog @@ -137,15 +146,78 @@ work rather than by a constant: of short strings, live for the length of one backup. This is the deliberate trade — one `List` per prefix instead of an `Exists` per object. - `metaLoader.cache`, where enabled, grows with the number of distinct - filemetas an operation touches. `backup`, `diff` and `prune` enable it - because they cross several snapshots, where an unchanged file keeps its - filemeta from one snapshot to the next and the same ref recurs. `ls`, - `restore` and `find` do not. - -`ls` is worth a note: it takes an uncached loader not to save memory but because -a cache there could never hit. A HAMT key derives from `meta.FileID`, which is -itself a `FileMeta` field, so no two keys share a filemeta ref and a single-root -walk reaches every ref exactly once. + filemetas an operation touches. Only `backup` and `diff` enable it, because + they are the two that read the same ref more than once — `diff` walks both + roots, and an unchanged file keeps its filemeta from one snapshot to the + next. `ls`, `restore`, `find` and `prune` read through. + +Two operations take an uncached loader for reasons worth stating, since both +look at first glance like they should memoize: + +- `ls` walks a single root. A HAMT key derives from `meta.FileID`, which is + itself a `FileMeta` field, so no two keys share a filemeta ref and the walk + reaches every ref exactly once. A cache could not hit. +- `prune` guards every load behind its `reachable` set — `markFileMeta` returns + early for a ref it has already marked — so it too reads each ref at most once + per run, no matter how many snapshots share it. This was measured, not + assumed: over four snapshots of one 2000-file tree the loader ended with 2000 + entries and zero hits. + +### Why an LRU is the wrong bound here + +The obvious fix for an unbounded cache is to cap it the way `NodeStore` caps +its node cache at 4096. Measurement says otherwise, and the reason generalises. + +`NodeStore` works under a bound because a HAMT descent re-touches the root and +the upper levels on every single lookup — real temporal locality, and a small +cache captures nearly all of it. A filemeta traversal has none: it sweeps each +ref exactly once per snapshot, uniformly. Under a cyclic sweep, LRU evicts +precisely the entry the next sweep is about to ask for, so the hit rate does +not degrade gracefully — it collapses. + +Hit rate over eight sweeps, from `BenchmarkMetaLoaderDiffPattern`: + +| working set | cache 1024 | cache 4096 | cache 16384 | +|---|---:|---:|---:| +| 1,000 files | 87.5% | 87.5% | 87.5% | +| 10,000 files | 0.0% | 0.0% | 87.5% | + +At 10,000 files a 4096-entry cache turns 10,000 store reads into 80,000. Any +fixed bound has this cliff; it only moves with the number. So the cache stays +unbounded, and the memory was taken out of the traversals instead — see below. + +### Measured footprints + +From `BenchmarkMetaLoaderRetained` and the tests in +`internal/engine/metaloader_memory_test.go`. "Before" is the state prior to the +two changes described above: `prune` memoizing, and `diff` retaining every entry +in its parent lookup rather than only folders. + +`prune`, heap retained after a run: + +| files | before | after | +|---|---:|---:| +| 5,000 | 3.3 MB | 1.3 MB | +| 20,000 | 12.7 MB | 2.9 MB | +| 50,000 | 28.1 MB | 4.0 MB | +| 100,000 | 54.6 MB | 6.7 MB | + +`diff`, peak heap while both parent lookups are live, on a tree with one folder +per twenty files: + +| files | parent entries before | after | peak before | peak after | +|---|---:|---:|---:|---:| +| 5,000 | 10,504 | 504 | 11.7 MB | 8.6 MB | +| 20,000 | 42,004 | 2,004 | 67.8 MB | 55.5 MB | +| 50,000 | 105,004 | 5,004 | 148.2 MB | 119.3 MB | +| 100,000 | 210,004 | 10,004 | 275.5 MB | 217.6 MB | + +Neither change costs a single extra store read, which is what distinguishes +them from bounding the cache. + +`diff`'s remaining peak is dominated by the loader cache, which is `O(tree)` by +design for the reason given above. Reducing it further means not walking both +roots in full, which is a change to the algorithm rather than to a cache. ## Invalidation diff --git a/internal/engine/diff.go b/internal/engine/diff.go index 3c8f57a..5684a14 100644 --- a/internal/engine/diff.go +++ b/internal/engine/diff.go @@ -188,6 +188,16 @@ func classifyEntry(d hamt.DiffEntry) (ChangeType, string) { } } +// collectMetadata builds the parent lookup toFileChange resolves paths through. +// +// It keeps folders and nothing else. The map is read at exactly one place — +// byID[parentID] in toFileChange — and a parent is a folder, so retaining a +// file's metadata here held a core.FileMeta per file that nothing ever read. +// On a 50k-file tree that was most of two maps. +// +// An entry naming a non-folder parent is not an error: collectMetaPaths already +// ends a chain at the deepest ancestor it can resolve, which is the most that +// can honestly be said about that entry's location. func (dm *DiffManager) collectMetadata(ctx context.Context, root string) (map[string]core.FileMeta, error) { byID := make(map[string]core.FileMeta) err := dm.tree.Walk(ctx, root, func(_, valueRef string) error { @@ -195,7 +205,9 @@ func (dm *DiffManager) collectMetadata(ctx context.Context, root string) (map[st if err != nil { return err } - byID[fm.FileID] = *fm + if fm.Type == core.FileTypeFolder { + byID[fm.FileID] = *fm + } return nil }) return byID, err diff --git a/internal/engine/metaloader_bench_test.go b/internal/engine/metaloader_bench_test.go new file mode 100644 index 0000000..bc92ca9 --- /dev/null +++ b/internal/engine/metaloader_bench_test.go @@ -0,0 +1,158 @@ +package engine + +import ( + "context" + "encoding/json" + "fmt" + "runtime" + "testing" + + "github.com/cloudstic/cli/internal/core" +) + +// populateMetas writes n distinct filemeta objects and returns their refs in +// insertion order. +func populateMetas(tb testing.TB, s *MockStore, n int) []string { + tb.Helper() + ctx := context.Background() + refs := make([]string, n) + for i := range n { + meta := core.FileMeta{ + Version: 1, + FileID: fmt.Sprintf("file-%d", i), + Name: fmt.Sprintf("document-%d.txt", i), + Type: core.FileTypeFile, + Parents: []string{fmt.Sprintf("folder-%d", i%128)}, + ContentHash: fmt.Sprintf("%064x", i), + Size: int64(i), + Mtime: int64(1700000000 + i), + } + ref, data, err := core.FileMetaRef(&meta) + if err != nil { + tb.Fatalf("FileMetaRef: %v", err) + } + if err := s.Put(ctx, ref, data); err != nil { + tb.Fatalf("Put: %v", err) + } + refs[i] = ref + } + return refs +} + +// retainedBytes reports the heap still held after fn returns, with the value it +// produced kept alive across the measurement. +func retainedBytes(fn func() any) uint64 { + runtime.GC() + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + + held := fn() + + runtime.GC() + runtime.ReadMemStats(&after) + runtime.KeepAlive(held) + + if after.HeapAlloc < before.HeapAlloc { + return 0 + } + return after.HeapAlloc - before.HeapAlloc +} + +// BenchmarkMetaLoaderRetained reports how much heap a loader still holds after +// reading n distinct filemetas. It is the data behind the files-to-memory table +// in docs/caching.md: a bounded loader must flatten as n grows, an unbounded one +// climbs linearly. +// +// The store is populated before measurement starts, so what is reported is the +// loader's own retention rather than the fixture's. +func BenchmarkMetaLoaderRetained(b *testing.B) { + for _, n := range []int{1000, 5000, 20000, 100000, 250000} { + b.Run(fmt.Sprintf("files=%d", n), func(b *testing.B) { + ctx := context.Background() + s := NewMockStore() + refs := populateMetas(b, s, n) + + var held uint64 + for b.Loop() { + held = retainedBytes(func() any { + l := newMetaLoader(s) + for _, ref := range refs { + if _, err := l.load(ctx, ref); err != nil { + b.Fatalf("load: %v", err) + } + } + return l + }) + } + + b.ReportMetric(float64(held)/1e6, "MB-retained") + b.ReportMetric(float64(held)/float64(n), "B/file") + }) + } +} + +// BenchmarkMetaLoaderDiffPattern exercises the access pattern the cache exists +// for: several consecutive snapshots over one tree, where an unchanged file +// keeps its filemeta and so recurs across every snapshot. churn is the fraction +// of files that change between snapshots. +// +// This is what sizing the cache depends on — a bound below the working set +// turns those repeats back into store reads. +func BenchmarkMetaLoaderDiffPattern(b *testing.B) { + const snapshots = 8 + for _, files := range []int{1000, 10000, 50000} { + b.Run(fmt.Sprintf("files=%d", files), func(b *testing.B) { + ctx := context.Background() + s := NewMockStore() + refs := populateMetas(b, s, files) + + counting := &countingStore{MockStore: s} + for b.Loop() { + counting.gets = 0 + l := newMetaLoader(counting) + for range snapshots { + for _, ref := range refs { + if _, err := l.load(ctx, ref); err != nil { + b.Fatalf("load: %v", err) + } + } + } + } + + total := files * snapshots + b.ReportMetric(float64(counting.gets), "store-reads") + b.ReportMetric(100*(1-float64(counting.gets)/float64(total)), "%hit") + }) + } +} + +// countingStore counts the reads that actually reach the store, which is how +// the cache's hit rate is observed from outside it. +type countingStore struct { + *MockStore + gets int +} + +func (s *countingStore) Get(ctx context.Context, key string) ([]byte, error) { + s.gets++ + return s.MockStore.Get(ctx, key) +} + +// sanity check that the fixture decodes, so a benchmark failure is never a +// broken fixture masquerading as a performance result. +func TestPopulateMetasRoundTrips(t *testing.T) { + s := NewMockStore() + refs := populateMetas(t, s, 4) + + data, err := s.Get(context.Background(), refs[2]) + if err != nil { + t.Fatalf("Get: %v", err) + } + var fm core.FileMeta + if err := json.Unmarshal(data, &fm); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if fm.Name != "document-2.txt" { + t.Errorf("Name = %q, want document-2.txt", fm.Name) + } +} diff --git a/internal/engine/metaloader_memory_test.go b/internal/engine/metaloader_memory_test.go new file mode 100644 index 0000000..3fcfb0b --- /dev/null +++ b/internal/engine/metaloader_memory_test.go @@ -0,0 +1,135 @@ +package engine + +import ( + "context" + "fmt" + "testing" + + "github.com/cloudstic/cli/internal/core" + "github.com/cloudstic/cli/internal/ui" +) + +// The two operations that walk a whole tree retain very different amounts of +// it, and which is which is a deliberate choice rather than an accident. These +// tests pin the choice, because both regressions are silent: a cache that stops +// being consulted still works, and one that starts retaining everything still +// returns the right answer. + +// prune reads each filemeta at most once — markFileMeta checks the reachable +// set before loading — so a memoizing loader could never return a hit and would +// cost a core.FileMeta per object in the repository. This fails if one is +// reintroduced. +func TestPruneLoaderDoesNotMemoize(t *testing.T) { + pm := NewPruneManager(Deps{Store: NewMockStore(), Reporter: ui.NewNoOpReporter()}) + if pm.metas.cache != nil { + t.Error("prune's filemeta loader memoizes; its reachable set already " + + "guarantees one load per ref, so the cache can only cost memory") + } +} + +// diff's parent lookup is consulted only as byID[parentID], and parents are +// folders. Retaining files there cost a core.FileMeta each for entries nothing +// read, which on a large tree was most of the map. +func TestDiffParentLookupHoldsOnlyFolders(t *testing.T) { + ctx := context.Background() + s := NewMockStore() + + folder := createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "dir", Name: "Documents", Type: core.FileTypeFolder, + }) + fileA := createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "a", Name: "a.txt", Parents: []string{"dir"}, ContentHash: "aa", + }) + fileB := createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "b", Name: "b.txt", Parents: []string{"dir"}, ContentHash: "bb", + }) + root := createHamt(ctx, t, s, []string{"dir", "a", "b"}, []string{folder, fileA, fileB}) + + dm := NewDiffManager(Deps{Store: s}) + byID, err := dm.collectMetadata(ctx, root) + if err != nil { + t.Fatalf("collectMetadata: %v", err) + } + + if len(byID) != 1 { + t.Errorf("parent lookup holds %d entries, want 1 (the folder); it retains "+ + "entries no caller reads", len(byID)) + } + if _, ok := byID["dir"]; !ok { + t.Error("parent lookup is missing the folder every file resolves through") + } +} + +// Filtering the parent lookup must not change any reported path — that is the +// whole point of the entries dropped being unread. +func TestDiffReportsFullPathsAfterFiltering(t *testing.T) { + ctx := context.Background() + s := NewMockStore() + + outer := createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "outer", Name: "Documents", Type: core.FileTypeFolder, + }) + inner := createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "inner", Name: "Photos", Type: core.FileTypeFolder, + Parents: []string{"outer"}, + }) + before := createHamt(ctx, t, s, []string{"outer", "inner"}, []string{outer, inner}) + + pic := createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "pic", Name: "pic.jpg", Parents: []string{"inner"}, ContentHash: "cc", + }) + after := createHamt(ctx, t, s, + []string{"outer", "inner", "pic"}, []string{outer, inner, pic}) + + r1 := saveSnapshot(ctx, s, &core.Snapshot{Seq: 1, Root: before, Created: "2025-01-01T00:00:00Z"}) + r2 := saveSnapshot(ctx, s, &core.Snapshot{Seq: 2, Root: after, Created: "2025-01-02T00:00:00Z"}) + + res, err := NewDiffManager(Deps{Store: s}).Run(ctx, r1, r2) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if len(res.Changes) != 1 { + t.Fatalf("got %d changes, want 1", len(res.Changes)) + } + if got := res.Changes[0].Path; got != "Documents/Photos/pic.jpg" { + t.Errorf("Path = %q, want Documents/Photos/pic.jpg — the nested path is "+ + "resolved through the parent lookup, so filtering it must not shorten it", got) + } +} + +// TestDiffRetentionGrowsWithFoldersNotFiles is the bounded-footprint claim +// stated as a test: adding files to a tree must not grow what diff retains, +// because only folders are kept. Entry counts are used rather than heap +// measurements, which are too noisy to assert on. +func TestDiffRetentionGrowsWithFoldersNotFiles(t *testing.T) { + build := func(files int) int { + ctx := context.Background() + s := NewMockStore() + + ids := []string{"dir"} + refs := []string{createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: "dir", Name: "Documents", Type: core.FileTypeFolder, + })} + for i := range files { + id := fmt.Sprintf("f%d", i) + ids = append(ids, id) + refs = append(refs, createMetaWithID(ctx, s, core.FileMeta{ + Version: 1, FileID: id, Name: id + ".txt", + Parents: []string{"dir"}, ContentHash: fmt.Sprintf("%02x", i), + })) + } + root := createHamt(ctx, t, s, ids, refs) + + byID, err := NewDiffManager(Deps{Store: s}).collectMetadata(ctx, root) + if err != nil { + t.Fatalf("collectMetadata: %v", err) + } + return len(byID) + } + + small, large := build(10), build(400) + if small != large { + t.Errorf("parent lookup grew from %d to %d entries when only the file count "+ + "changed; retention must track folders, not files", small, large) + } +} diff --git a/internal/engine/prune.go b/internal/engine/prune.go index 872ec4b..d755311 100644 --- a/internal/engine/prune.go +++ b/internal/engine/prune.go @@ -45,7 +45,10 @@ func NewPruneManager(d Deps) *PruneManager { store: meteredStore, tree: hamt.NewTree(meteredStore), reporter: d.Reporter, - metas: newMetaLoader(meteredStore), + // Uncached: markFileMeta guards every load behind the reachable set, so + // a ref is read at most once per run and a cache could never hit. Holding + // one would cost a core.FileMeta per object in the repository for nothing. + metas: newUncachedMetaLoader(meteredStore), } } From 817a4b0bdfd5191b0c67454d79da91a7abcb6891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hermann?= Date: Tue, 4 Aug 2026 09:37:56 +0200 Subject: [PATCH 2/2] refactor(engine): name diff's folder index for what it holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/engine/diff.go | 37 +++++++++++++---------- internal/engine/metaloader_memory_test.go | 18 +++++------ 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/internal/engine/diff.go b/internal/engine/diff.go index 5684a14..e33b558 100644 --- a/internal/engine/diff.go +++ b/internal/engine/diff.go @@ -136,17 +136,17 @@ func (dm *DiffManager) loadSnapshot(ctx context.Context, ref string) (*core.Snap func (dm *DiffManager) diffRoots(ctx context.Context, root1, root2 string) ([]FileChange, error) { var changes []FileChange - oldByID, err := dm.collectMetadata(ctx, root1) + oldFolderByID, err := dm.collectFolders(ctx, root1) if err != nil { return nil, err } - newByID, err := dm.collectMetadata(ctx, root2) + newFolderByID, err := dm.collectFolders(ctx, root2) if err != nil { return nil, err } err = dm.tree.Diff(ctx, root1, root2, func(d hamt.DiffEntry) error { - change, err := dm.toFileChange(ctx, d, oldByID, newByID) + change, err := dm.toFileChange(ctx, d, oldFolderByID, newFolderByID) if err != nil { return err } @@ -156,21 +156,21 @@ func (dm *DiffManager) diffRoots(ctx context.Context, root1, root2 string) ([]Fi return changes, err } -func (dm *DiffManager) toFileChange(ctx context.Context, d hamt.DiffEntry, oldByID, newByID map[string]core.FileMeta) (FileChange, error) { +func (dm *DiffManager) toFileChange(ctx context.Context, d hamt.DiffEntry, oldFolderByID, newFolderByID map[string]core.FileMeta) (FileChange, error) { ct, metaRef := classifyEntry(d) meta, err := dm.metas.load(ctx, metaRef) if err != nil { return FileChange{}, err } - byID := newByID + folderByID := newFolderByID if ct == ChangeRemoved { - byID = oldByID + folderByID = oldFolderByID } return FileChange{ Type: ct, Path: fileMetaPath(*meta, func(parentID string) (core.FileMeta, bool) { - parent, ok := byID[parentID] + parent, ok := folderByID[parentID] return parent, ok }), Meta: *meta, @@ -188,27 +188,32 @@ func classifyEntry(d hamt.DiffEntry) (ChangeType, string) { } } -// collectMetadata builds the parent lookup toFileChange resolves paths through. +// collectFolders indexes root's folders by FileID, which is the lookup +// toFileChange resolves a change's path through. // -// It keeps folders and nothing else. The map is read at exactly one place — -// byID[parentID] in toFileChange — and a parent is a folder, so retaining a -// file's metadata here held a core.FileMeta per file that nothing ever read. -// On a 50k-file tree that was most of two maps. +// It keeps folders and nothing else, because that is all anyone asks it for: +// the map is read at exactly one place — folderByID[parentID] in toFileChange — +// and a parent is a folder. Retaining a file's metadata here held a +// core.FileMeta per file that nothing ever read, which on a 100k-file tree was +// 200,000 of the 210,004 entries across the two maps. +// +// Note that RestoreManager.collectMetadata, which looks like this one, is +// deliberately not the same: restore writes every entry, so it needs them all. // // An entry naming a non-folder parent is not an error: collectMetaPaths already // ends a chain at the deepest ancestor it can resolve, which is the most that // can honestly be said about that entry's location. -func (dm *DiffManager) collectMetadata(ctx context.Context, root string) (map[string]core.FileMeta, error) { - byID := make(map[string]core.FileMeta) +func (dm *DiffManager) collectFolders(ctx context.Context, root string) (map[string]core.FileMeta, error) { + folderByID := make(map[string]core.FileMeta) err := dm.tree.Walk(ctx, root, func(_, valueRef string) error { fm, err := dm.metas.load(ctx, valueRef) if err != nil { return err } if fm.Type == core.FileTypeFolder { - byID[fm.FileID] = *fm + folderByID[fm.FileID] = *fm } return nil }) - return byID, err + return folderByID, err } diff --git a/internal/engine/metaloader_memory_test.go b/internal/engine/metaloader_memory_test.go index 3fcfb0b..3473215 100644 --- a/internal/engine/metaloader_memory_test.go +++ b/internal/engine/metaloader_memory_test.go @@ -27,7 +27,7 @@ func TestPruneLoaderDoesNotMemoize(t *testing.T) { } } -// diff's parent lookup is consulted only as byID[parentID], and parents are +// diff's parent lookup is consulted only as folderByID[parentID], and parents are // folders. Retaining files there cost a core.FileMeta each for entries nothing // read, which on a large tree was most of the map. func TestDiffParentLookupHoldsOnlyFolders(t *testing.T) { @@ -46,16 +46,16 @@ func TestDiffParentLookupHoldsOnlyFolders(t *testing.T) { root := createHamt(ctx, t, s, []string{"dir", "a", "b"}, []string{folder, fileA, fileB}) dm := NewDiffManager(Deps{Store: s}) - byID, err := dm.collectMetadata(ctx, root) + folderByID, err := dm.collectFolders(ctx, root) if err != nil { - t.Fatalf("collectMetadata: %v", err) + t.Fatalf("collectFolders: %v", err) } - if len(byID) != 1 { + if len(folderByID) != 1 { t.Errorf("parent lookup holds %d entries, want 1 (the folder); it retains "+ - "entries no caller reads", len(byID)) + "entries no caller reads", len(folderByID)) } - if _, ok := byID["dir"]; !ok { + if _, ok := folderByID["dir"]; !ok { t.Error("parent lookup is missing the folder every file resolves through") } } @@ -120,11 +120,11 @@ func TestDiffRetentionGrowsWithFoldersNotFiles(t *testing.T) { } root := createHamt(ctx, t, s, ids, refs) - byID, err := NewDiffManager(Deps{Store: s}).collectMetadata(ctx, root) + folderByID, err := NewDiffManager(Deps{Store: s}).collectFolders(ctx, root) if err != nil { - t.Fatalf("collectMetadata: %v", err) + t.Fatalf("collectFolders: %v", err) } - return len(byID) + return len(folderByID) } small, large := build(10), build(400)