perf(engine): stop retaining filemetas nothing reads

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
 

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,15 +188,32 @@ func classifyEntry(d hamt.DiffEntry) (ChangeType, string) {
 	}
 }
 
-func (dm *DiffManager) collectMetadata(ctx context.Context, root string) (map[string]core.FileMeta, error) {
-	byID := make(map[string]core.FileMeta)
+// collectFolders indexes root's folders by FileID, which is the lookup
+// toFileChange resolves a change's path through.
+//
+// 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) 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
 		}
-		byID[fm.FileID] = *fm
+		if fm.Type == core.FileTypeFolder {
+			folderByID[fm.FileID] = *fm
+		}
 		return nil
 	})
-	return byID, err
+	return folderByID, err
 }

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)
+	}
+}