perf(store): size the pack body cache to the working set

internal/storelayer/pack.go

@@ -72,7 +72,7 @@ type PackStore struct {
 	pendingKeys map[string]struct{}
 
 	// LRU cache for recently downloaded packfiles to accelerate Get() and HAMT walks
-	packCache *lru.Cache[string, []byte]
+	packCache *packBodyCache
 
 	// Misses per pack since it was last cached, used to decide when reading one
 	// object at a time is no longer the cheaper option. See resolveFromPack.
@@ -107,7 +107,7 @@ func WithPackIndexKey(key []byte) PackOption {
 // NewPackStore initializes a new MicroPackStore over an existing store.ObjectStore.
 func NewPackStore(inner store.ObjectStore, opts ...PackOption) (*PackStore, error) {
 	// Keep up to 30 MB of packfiles in memory (around 4 packs) to speed up reads
-	cache, err := lru.New[string, []byte](4)
+	cache, err := newPackBodyCache(packBodyCacheBudget)
 	if err != nil {
 		return nil, fmt.Errorf("pack cache init: %w", err)
 	}
@@ -132,7 +132,7 @@ func NewPackStore(inner store.ObjectStore, opts ...PackOption) (*PackStore, erro
 	}
 	// Logged after the options are applied so it reaches the sink the caller
 	// asked for rather than the process-wide fallback.
-	s.debugf("init packstore: LRU size=%d", 4)
+	s.debugf("init packstore: pack body budget=%d bytes", packBodyCacheBudget)
 	return s, nil
 }
 

internal/storelayer/packbodycache.go

@@ -0,0 +1,110 @@
+package storelayer
+
+import (
+	"sync"
+
+	lru "github.com/hashicorp/golang-lru/v2"
+)
+
+// packBodyCacheBudget is how many bytes of packfile bodies PackStore keeps in
+// memory.
+//
+// The cache held a fixed count of four packs, which was two problems. Its real
+// size was that count times whatever a pack weighed — up to maxPackSize plus a
+// footer each, a figure nobody stated. And four was below the working set of an
+// ordinary repository, which is what made a miss expensive enough to matter: a
+// miss re-reads an entire packfile to return one small object.
+//
+// Measured on a 50,000-file repository with two snapshots and 6 packs, `check`
+// issues 127,789 lookups and misses 0.39% of them against a four-pack cache —
+// 495 misses, each re-reading ~9 MB, which is the 4.2 GB of transfer that made
+// `check` allocate 3 GB in the benchmark. At six packs the same trace misses six
+// times. The access order is already good; the cache was simply smaller than
+// what the order needs.
+//
+// Eight packs' worth is the budget. It covers the working set of a repository in
+// the size range the benchmark exercises while staying a stated ceiling rather
+// than an emergent product, and a repository with smaller packs gets
+// proportionally more of them — which a count-based cache could not express.
+//
+// This is deliberately larger than what it replaces. Going the other way was
+// measured and is worse: at a 32 MB budget the same `check` peaks at 251 MB
+// against 167 MB, because the transfers a smaller cache forces cost more than
+// the residency it saves.
+const packBodyCacheBudget = 8 * maxPackSize
+
+// packBodyCache is an LRU of packfile bodies bounded by total bytes.
+//
+// hashicorp/golang-lru bounds by entry count, so the byte accounting lives here:
+// the underlying cache is given a generous entry limit and this type evicts from
+// the tail until the budget is met.
+type packBodyCache struct {
+	mu     sync.Mutex
+	lru    *lru.Cache[string, []byte]
+	bytes  int
+	budget int
+}
+
+// newPackBodyCache returns a cache holding at most budget bytes of pack bodies.
+func newPackBodyCache(budget int) (*packBodyCache, error) {
+	c := &packBodyCache{budget: budget}
+	// The entry limit only has to be beyond what the budget can hold. A pack is
+	// never smaller than one object, so this cannot be reached before the byte
+	// budget is.
+	inner, err := lru.NewWithEvict[string, []byte](1<<20, func(_ string, v []byte) {
+		c.bytes -= len(v)
+	})
+	if err != nil {
+		return nil, err
+	}
+	c.lru = inner
+	return c, nil
+}
+
+func (c *packBodyCache) Get(key string) ([]byte, bool) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return c.lru.Get(key)
+}
+
+// Add stores data under key and evicts from the tail until the cache is within
+// budget.
+//
+// A single body larger than the whole budget is still cached, alone. Refusing it
+// would mean re-reading that pack for every object taken out of it, which is the
+// cost this cache exists to avoid.
+func (c *packBodyCache) Add(key string, data []byte) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	if _, existing := c.lru.Peek(key); existing {
+		// The eviction callback adjusts the count for the value being replaced.
+		c.lru.Remove(key)
+	}
+	c.lru.Add(key, data)
+	c.bytes += len(data)
+
+	for c.bytes > c.budget && c.lru.Len() > 1 {
+		c.lru.RemoveOldest()
+	}
+}
+
+func (c *packBodyCache) Remove(key string) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.lru.Remove(key)
+}
+
+// byteLen reports the bytes currently held, for tests that pin the bound.
+func (c *packBodyCache) byteLen() int {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return c.bytes
+}
+
+// len reports how many bodies are held, for tests.
+func (c *packBodyCache) len() int {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return c.lru.Len()
+}

internal/storelayer/packbodycache_test.go

@@ -0,0 +1,155 @@
+package storelayer
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/cloudstic/cli/pkg/store/local"
+)
+
+// The bound is the point: whatever the size of the bodies, the cache holds at
+// most its budget. A fixed entry count could not say that.
+func TestPackBodyCache_StaysWithinItsByteBudget(t *testing.T) {
+	const budget = 1000
+	c, err := newPackBodyCache(budget)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for i := 0; i < 50; i++ {
+		c.Add(fmt.Sprintf("packs/%d", i), bytes.Repeat([]byte("x"), 300))
+		if got := c.byteLen(); got > budget {
+			t.Fatalf("after %d adds the cache holds %d bytes, over the %d budget", i+1, got, budget)
+		}
+	}
+	if c.len() == 0 {
+		t.Error("everything was evicted")
+	}
+}
+
+// Smaller bodies mean more of them cached — behaviour a fixed count cannot
+// express, since it held four packs whether they were 512 KB or 8 MB.
+func TestPackBodyCache_HoldsMoreOfSmallerBodies(t *testing.T) {
+	fill := func(size int) int {
+		c, err := newPackBodyCache(8000)
+		if err != nil {
+			t.Fatal(err)
+		}
+		for i := 0; i < 100; i++ {
+			c.Add(fmt.Sprintf("packs/%d", i), bytes.Repeat([]byte("x"), size))
+		}
+		return c.len()
+	}
+	if small, large := fill(200), fill(2000); small <= large {
+		t.Errorf("cached %d small bodies and %d large; smaller should fit in greater number", small, large)
+	}
+}
+
+// A body larger than the whole budget is still served, alone.
+func TestPackBodyCache_KeepsABodyLargerThanTheBudget(t *testing.T) {
+	c, err := newPackBodyCache(100)
+	if err != nil {
+		t.Fatal(err)
+	}
+	big := bytes.Repeat([]byte("y"), 5000)
+	c.Add("packs/big", big)
+	if got, ok := c.Get("packs/big"); !ok || len(got) != len(big) {
+		t.Fatalf("oversized body: got %d bytes, ok=%v", len(got), ok)
+	}
+}
+
+func TestPackBodyCache_ReplaceAndRemoveAccountForBytes(t *testing.T) {
+	c, err := newPackBodyCache(10000)
+	if err != nil {
+		t.Fatal(err)
+	}
+	data := bytes.Repeat([]byte("z"), 400)
+	for i := 0; i < 5; i++ {
+		c.Add("packs/same", data)
+	}
+	if got := c.byteLen(); got != len(data) {
+		t.Fatalf("holds %d bytes for one entry of %d", got, len(data))
+	}
+	c.Add("packs/other", data)
+	c.Remove("packs/same")
+	if got := c.byteLen(); got != len(data) {
+		t.Fatalf("holds %d bytes after removing one of two %d-byte entries", got, len(data))
+	}
+}
+
+// The regression this exists for (#458): reading across more packs than the old
+// four-entry cache held made every pass re-read whole packfiles. With a budget
+// that covers the working set, each pack is transferred about once.
+func TestPackStore_DoesNotRereadPacksWhenTheWorkingSetFits(t *testing.T) {
+	ctx := context.Background()
+
+	base, err := local.New(t.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	counting := &countingRangeStore{ObjectStore: base}
+	writer, err := NewPackStore(counting)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Eight packs, more than the four the count-based cache held.
+	// More objects per pack than packPromoteAfter, so a pack is cached partway
+	// through the first pass rather than being ranged-read for the whole test.
+	const packs = 8
+	var keys []string
+	payload := bytes.Repeat([]byte("p"), 4*1024)
+	for p := 0; p < packs; p++ {
+		for i := 0; i < 3*packPromoteAfter; i++ {
+			key := fmt.Sprintf("filemeta/%064x", p*100+i)
+			if err := writer.Put(ctx, key, payload); err != nil {
+				t.Fatal(err)
+			}
+			keys = append(keys, key)
+		}
+		if err := writer.Flush(ctx); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	reader, err := NewPackStore(counting)
+	if err != nil {
+		t.Fatal(err)
+	}
+	counting.fullGets, counting.rangeGets, counting.bytesRead = 0, 0, 0
+
+	// Three passes over every object, which is what makes a too-small cache
+	// visible: the second and third passes should be almost entirely hits.
+	for pass := 0; pass < 3; pass++ {
+		for _, key := range keys {
+			if _, err := reader.Get(ctx, key); err != nil {
+				t.Fatalf("pass %d, Get(%s): %v", pass, key, err)
+			}
+		}
+	}
+
+	// Assert on bytes, not on whole-pack count. A too-small cache does not
+	// simply do more whole-pack transfers — ranged reads absorb most of the
+	// extra misses (#452), so the count barely moves while the traffic does.
+	stored, err := base.List(ctx, packPrefix)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var packBytes int64
+	for _, ref := range stored {
+		n, err := base.Size(ctx, ref)
+		if err != nil {
+			t.Fatal(err)
+		}
+		packBytes += n
+	}
+
+	// Three passes over a working set that fits should transfer each pack about
+	// once. Allowing double leaves room for the misses before promotion caches
+	// a pack, without leaving room for re-reading the set on every pass.
+	if limit := 2 * packBytes; counting.bytesRead > limit {
+		t.Errorf("read %d bytes across three passes over %d bytes of packs (limit %d); the working set is being re-read",
+			counting.bytesRead, packBytes, limit)
+	}
+}