diff --git a/cs b/cs index febda0b..75df4d4 100755 Binary files a/cs and b/cs differ diff --git a/internal/storelayer/pack.go b/internal/storelayer/pack.go index ac0971f..66667e1 100644 --- a/internal/storelayer/pack.go +++ b/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 } diff --git a/internal/storelayer/packbodycache.go b/internal/storelayer/packbodycache.go new file mode 100644 index 0000000..02e2b9f --- /dev/null +++ b/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() +} diff --git a/internal/storelayer/packbodycache_test.go b/internal/storelayer/packbodycache_test.go new file mode 100644 index 0000000..447110a --- /dev/null +++ b/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) + } +}