From d100c06f4db7927abaa7984df1f391cc175f5f58 Mon Sep 17 00:00:00 2001 From: Cody Littley Date: Thu, 10 Sep 2026 10:59:14 -0500 Subject: [PATCH 1/2] better lock parallelism for views --- sei-db/common/structures/lru_queue.go | 98 ----- sei-db/common/structures/lru_queue_test.go | 310 --------------- sei-db/db_engine/view/read_cache.go | 382 +++++++++++-------- sei-db/db_engine/view/read_cache_test.go | 61 ++- sei-db/db_engine/view/shard.go | 282 +++++++++----- sei-db/db_engine/view/view_iterator_test.go | 2 +- sei-db/db_engine/view/view_manager.go | 6 +- sei-db/db_engine/view/view_manager_config.go | 18 + sei-db/db_engine/view/view_manager_impl.go | 28 +- 9 files changed, 511 insertions(+), 676 deletions(-) delete mode 100644 sei-db/common/structures/lru_queue.go delete mode 100644 sei-db/common/structures/lru_queue_test.go diff --git a/sei-db/common/structures/lru_queue.go b/sei-db/common/structures/lru_queue.go deleted file mode 100644 index b704f3485c..0000000000 --- a/sei-db/common/structures/lru_queue.go +++ /dev/null @@ -1,98 +0,0 @@ -package structures - -import ( - "container/list" - "fmt" -) - -// LRUQueue implements a queue-like abstraction with LRU semantics, tracking both the number of -// entries and their aggregate size. Not thread safe. -type LRUQueue struct { - order *list.List - entries map[string]*list.Element - totalSize uint64 -} - -type lruQueueEntry struct { - key string - size uint64 -} - -// NewLRUQueue creates a new LRU queue. -func NewLRUQueue() *LRUQueue { - return &LRUQueue{ - order: list.New(), - entries: make(map[string]*list.Element), - } -} - -// Push adds a new entry to the LRU queue. Can also be used to update an existing value with a new weight. -func (lru *LRUQueue) Push( - // the key that was recently interacted with - key []byte, - // the size of the key + value - size uint64, -) { - if elem, ok := lru.entries[string(key)]; ok { - entry := elem.Value.(*lruQueueEntry) - if lru.totalSize < entry.size { - // should be impossible - panic(fmt.Errorf("size tracking is corrupted: totalSize %d < entry.size %d", - lru.totalSize, entry.size)) - } - lru.totalSize -= entry.size - lru.totalSize += size - entry.size = size - lru.order.MoveToBack(elem) - return - } - - keyStr := string(key) - elem := lru.order.PushBack(&lruQueueEntry{ - key: keyStr, - size: size, - }) - lru.entries[keyStr] = elem - lru.totalSize += size -} - -// Touch signals that an entry has been interacted with, moving it to the back of the queue -// (i.e. making it so it doesn't get popped soon). -func (lru *LRUQueue) Touch(key []byte) { - elem, ok := lru.entries[string(key)] - if !ok { - return - } - lru.order.MoveToBack(elem) -} - -// GetTotalSize returns the total size of all entries in the LRU queue. -func (lru *LRUQueue) GetTotalSize() uint64 { - return lru.totalSize -} - -// GetCount returns a count of the number of entries in the LRU queue, where each entry counts for 1 -// regardless of size. -func (lru *LRUQueue) GetCount() uint64 { - return uint64(len(lru.entries)) -} - -// PopLeastRecentlyUsed pops a single element out of the queue. The element removed is the entry -// least recently passed to Push/Touch. Returns the key in string form to avoid copying the key an -// additional time. Panics if the queue is empty. -func (lru *LRUQueue) PopLeastRecentlyUsed() string { - elem := lru.order.Front() - if elem == nil { - panic("cannot pop from empty LRU queue") - } - - lru.order.Remove(elem) - entry := elem.Value.(*lruQueueEntry) - delete(lru.entries, entry.key) - if entry.size > lru.totalSize { - // should be impossible - panic(fmt.Errorf("size tracking is corrupted: entry.size %d > totalSize %d", entry.size, lru.totalSize)) - } - lru.totalSize -= entry.size - return entry.key -} diff --git a/sei-db/common/structures/lru_queue_test.go b/sei-db/common/structures/lru_queue_test.go deleted file mode 100644 index 3d8ad43051..0000000000 --- a/sei-db/common/structures/lru_queue_test.go +++ /dev/null @@ -1,310 +0,0 @@ -package structures - -import ( - "fmt" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestLRUQueueIsolatesFromCallerMutation(t *testing.T) { - lru := NewLRUQueue() - - key := []byte("a") - lru.Push(key, 1) - key[0] = 'z' - - require.Equal(t, "a", lru.PopLeastRecentlyUsed()) -} - -func TestNewLRUQueueStartsEmpty(t *testing.T) { - lru := NewLRUQueue() - - require.Equal(t, uint64(0), lru.GetCount()) - require.Equal(t, uint64(0), lru.GetTotalSize()) -} - -func TestPopLeastRecentlyUsedPanicsOnEmptyQueue(t *testing.T) { - lru := NewLRUQueue() - require.Panics(t, func() { lru.PopLeastRecentlyUsed() }) -} - -func TestPopLeastRecentlyUsedPanicsAfterDrain(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("x"), 1) - lru.PopLeastRecentlyUsed() - - require.Panics(t, func() { lru.PopLeastRecentlyUsed() }) -} - -func TestPushSingleElement(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("only"), 42) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, uint64(42), lru.GetTotalSize()) - require.Equal(t, "only", lru.PopLeastRecentlyUsed()) -} - -func TestPushDuplicateDecreasesSize(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("k"), 100) - lru.Push([]byte("k"), 30) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, uint64(30), lru.GetTotalSize()) -} - -func TestPushDuplicateMovesToBack(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 1) - lru.Push([]byte("b"), 1) - lru.Push([]byte("c"), 1) - - // Re-push "a" — should move it behind "b" and "c" - lru.Push([]byte("a"), 1) - - require.Equal(t, "b", lru.PopLeastRecentlyUsed()) - require.Equal(t, "c", lru.PopLeastRecentlyUsed()) - require.Equal(t, "a", lru.PopLeastRecentlyUsed()) -} - -func TestPushZeroSize(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("z"), 0) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, uint64(0), lru.GetTotalSize()) - require.Equal(t, "z", lru.PopLeastRecentlyUsed()) - require.Equal(t, uint64(0), lru.GetTotalSize()) -} - -func TestPushEmptyKey(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte(""), 5) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, "", lru.PopLeastRecentlyUsed()) -} - -func TestPushRepeatedUpdatesToSameKey(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("k"), 1) - lru.Push([]byte("k"), 2) - lru.Push([]byte("k"), 3) - lru.Push([]byte("k"), 4) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, uint64(4), lru.GetTotalSize()) -} - -func TestTouchNonexistentKeyIsNoop(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 1) - - lru.Touch([]byte("missing")) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, "a", lru.PopLeastRecentlyUsed()) -} - -func TestTouchOnEmptyQueueIsNoop(t *testing.T) { - lru := NewLRUQueue() - lru.Touch([]byte("ghost")) - - require.Equal(t, uint64(0), lru.GetCount()) -} - -func TestTouchSingleElement(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("solo"), 10) - lru.Touch([]byte("solo")) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, "solo", lru.PopLeastRecentlyUsed()) -} - -func TestTouchDoesNotAffectSizeOrCount(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 3) - lru.Push([]byte("b"), 7) - - lru.Touch([]byte("a")) - - require.Equal(t, uint64(2), lru.GetCount()) - require.Equal(t, uint64(10), lru.GetTotalSize()) -} - -func TestMultipleTouchesChangeOrder(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 1) - lru.Push([]byte("b"), 1) - lru.Push([]byte("c"), 1) - - // Order: a, b, c - lru.Touch([]byte("a")) // Order: b, c, a - lru.Touch([]byte("b")) // Order: c, a, b - - require.Equal(t, "c", lru.PopLeastRecentlyUsed()) - require.Equal(t, "a", lru.PopLeastRecentlyUsed()) - require.Equal(t, "b", lru.PopLeastRecentlyUsed()) -} - -func TestTouchAlreadyMostRecentIsNoop(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 1) - lru.Push([]byte("b"), 1) - - lru.Touch([]byte("b")) // "b" is already at back - - require.Equal(t, "a", lru.PopLeastRecentlyUsed()) - require.Equal(t, "b", lru.PopLeastRecentlyUsed()) -} - -func TestPopDecrementsCountAndSize(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 10) - lru.Push([]byte("b"), 20) - lru.Push([]byte("c"), 30) - - lru.PopLeastRecentlyUsed() - - require.Equal(t, uint64(2), lru.GetCount()) - require.Equal(t, uint64(50), lru.GetTotalSize()) - - lru.PopLeastRecentlyUsed() - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, uint64(30), lru.GetTotalSize()) -} - -func TestPopFIFOOrderWithoutTouches(t *testing.T) { - lru := NewLRUQueue() - keys := []string{"first", "second", "third", "fourth"} - for _, k := range keys { - lru.Push([]byte(k), 1) - } - - for _, want := range keys { - require.Equal(t, want, lru.PopLeastRecentlyUsed()) - } -} - -func TestPushAfterDrain(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 5) - lru.PopLeastRecentlyUsed() - - lru.Push([]byte("x"), 10) - lru.Push([]byte("y"), 20) - - require.Equal(t, uint64(2), lru.GetCount()) - require.Equal(t, uint64(30), lru.GetTotalSize()) - require.Equal(t, "x", lru.PopLeastRecentlyUsed()) -} - -func TestPushPreviouslyPoppedKey(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("recycled"), 5) - lru.PopLeastRecentlyUsed() - - lru.Push([]byte("recycled"), 99) - - require.Equal(t, uint64(1), lru.GetCount()) - require.Equal(t, uint64(99), lru.GetTotalSize()) - require.Equal(t, "recycled", lru.PopLeastRecentlyUsed()) -} - -func TestInterleavedPushAndPop(t *testing.T) { - lru := NewLRUQueue() - - lru.Push([]byte("a"), 1) - lru.Push([]byte("b"), 2) - - require.Equal(t, "a", lru.PopLeastRecentlyUsed()) - - lru.Push([]byte("c"), 3) - - require.Equal(t, uint64(2), lru.GetCount()) - require.Equal(t, uint64(5), lru.GetTotalSize()) - require.Equal(t, "b", lru.PopLeastRecentlyUsed()) - require.Equal(t, "c", lru.PopLeastRecentlyUsed()) -} - -func TestTouchThenPushSameKey(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 1) - lru.Push([]byte("b"), 1) - - lru.Touch([]byte("a")) // order: b, a - lru.Push([]byte("a"), 50) // updates size, stays at back - - require.Equal(t, uint64(2), lru.GetCount()) - require.Equal(t, uint64(51), lru.GetTotalSize()) - require.Equal(t, "b", lru.PopLeastRecentlyUsed()) -} - -func TestBinaryKeyData(t *testing.T) { - lru := NewLRUQueue() - k1 := []byte{0x00, 0xFF, 0x01} - k2 := []byte{0x00, 0xFF, 0x02} - - lru.Push(k1, 10) - lru.Push(k2, 20) - - require.Equal(t, uint64(2), lru.GetCount()) - require.Equal(t, string(k1), lru.PopLeastRecentlyUsed()) - - lru.Touch(k2) - require.Equal(t, string(k2), lru.PopLeastRecentlyUsed()) -} - -func TestCallerMutationAfterTouchDoesNotAffectQueue(t *testing.T) { - lru := NewLRUQueue() - key := []byte("abc") - lru.Push(key, 1) - - key[0] = 'Z' - lru.Touch(key) // Touch with mutated key ("Zbc") — should be a no-op - - require.Equal(t, "abc", lru.PopLeastRecentlyUsed()) -} - -func TestManyEntries(t *testing.T) { - lru := NewLRUQueue() - n := 1000 - var totalSize uint64 - - for i := 0; i < n; i++ { - k := fmt.Sprintf("key-%04d", i) - lru.Push([]byte(k), uint64(i+1)) - totalSize += uint64(i + 1) - } - - require.Equal(t, uint64(n), lru.GetCount()) - require.Equal(t, totalSize, lru.GetTotalSize()) - - for i := 0; i < n; i++ { - want := fmt.Sprintf("key-%04d", i) - require.Equal(t, want, lru.PopLeastRecentlyUsed(), "pop %d", i) - } - - require.Equal(t, uint64(0), lru.GetCount()) - require.Equal(t, uint64(0), lru.GetTotalSize()) -} - -func TestPushUpdatedSizeThenPopVerifySizeAccounting(t *testing.T) { - lru := NewLRUQueue() - lru.Push([]byte("a"), 10) - lru.Push([]byte("b"), 20) - lru.Push([]byte("a"), 5) // decrease a's size from 10 to 5 - - require.Equal(t, uint64(25), lru.GetTotalSize()) - - // Pop "b" (it's the LRU since "a" was re-pushed to back). - lru.PopLeastRecentlyUsed() - require.Equal(t, uint64(5), lru.GetTotalSize()) - - lru.PopLeastRecentlyUsed() - require.Equal(t, uint64(0), lru.GetTotalSize()) -} diff --git a/sei-db/db_engine/view/read_cache.go b/sei-db/db_engine/view/read_cache.go index 990185955c..831ae87b86 100644 --- a/sei-db/db_engine/view/read_cache.go +++ b/sei-db/db_engine/view/read_cache.go @@ -4,55 +4,38 @@ import ( "context" "errors" "fmt" + "math" "sync" + "sync/atomic" "time" errorutils "github.com/sei-protocol/sei-chain/sei-db/common/errors" - "github.com/sei-protocol/sei-chain/sei-db/common/structures" "github.com/sei-protocol/sei-chain/sei-db/common/threading" "github.com/sei-protocol/sei-chain/sei-db/db_engine/types" ) -/* -This implementation currently uses a single exclusive lock, as opposed to a RW lock. This is a lot simpler than -using a RW lock, but it comes at higher risk of contention under certain workloads. If this contention ever -becomes a problem, we might consider switching to a RW lock. Below is a potential implementation strategy -for converting to a RW lock: - -- Create a background goroutine that is responsible for LRU eviction and updating the LRU. -- The eviction goroutine should periodically wake up, grab the lock, and do eviction. -- When Get() is called, the calling goroutine should grab a read lock and attempt to read the value. - - If the value is present, send a message to the eviction goroutine over a channel (so it can update the LRU) - and return the value. In this way, many readers can read from this shard concurrently. - - If the value is missing, drop the read lock and acquire a write lock. Then, handle the read - like we currently handle in the current implementation. -*/ - -// readCache is a read-through cache over the backing DB. It knows nothing about versions or -// views; the shard resolves versioned data first and consults the cache only for keys with no -// in-memory override. +// readCache is a read-through cache over the backing DB. Eviction order is approximate, and the cache +// is guarded by its shard's lock. // -// A failed DB read is fatal: the cache bricks the manager, which takes every shard out of service so -// no further reads are served (see outOfServiceErr). +// Capitalized methods are the surface the shard calls; readCache is unexported, so they are not exports. // -// The cache is a passive component of its shard and shares the shard's mutex: it holds no lock -// of its own. Methods with the Locked postfix require the shared lock to be held and never -// block; resolve and resolveBatch run without the lock and may block on DB reads; the -// background read-completion paths (injectValue, bulkInjectValues) acquire the lock themselves -// for a single self-contained section each. Keeping one mutex preserves the manager's -// single-lock-grab read path. +// Method postfixes state the lock contract: RLocked and WLocked require the caller to hold the read or +// write lock, Unlocked requires the caller to hold neither, and a bare name has no lock dependency. type readCache struct { // Cancelled when the manager shuts down; interrupts blocked waits on in-flight reads. ctx context.Context + // The manager's configuration. The cache reads only the fields that concern it. + config *ViewManagerConfig + // The underlying key-value database. db types.KeyValueDB // A pool for asynchronous reads. readPool threading.Pool - // The shard's mutex, shared with this cache (see the type doc). - lock *sync.Mutex + // The shard's lock, borrowed by this cache. + lock *sync.RWMutex // Maps the context cancellation observed by a blocked read to the manager's shutdown error: // the latched fatal error, or ErrViewManagerClosed on a clean close. Blocked reads select on ctx, @@ -67,26 +50,28 @@ type readCache struct { // The failure that took this cache out of service, or nil while it is healthy. Set when the // manager bricks — for any reason, not only a failed read of this shard — after which the shard // refuses reads rather than serving data the manager can no longer vouch for. - // - // Guarded by the shared lock. outOfServiceErr error // ViewManager-level metrics. Nil-safe; if nil, no metrics are recorded. metrics *ViewManagerMetrics - // The estimated bookkeeping overhead per entry, in bytes, counted toward the size budget. - overheadPerEntry uint64 - // The maximum size of the cache, in bytes. maxSize uint64 // The cached entries, keyed by string(key). entries map[string]*cacheEntry - // Organizes entries for LRU eviction. Only entries in a terminal data state - // (available/deleted) are in the queue: scheduled entries have no value yet, and failed - // entries have no value to serve. - gcQueue *structures.LRUQueue + // The number of bytes counted toward the size budget. Only entries in a terminal data state + // (available/deleted) are counted. + trackedBytes uint64 + + // The number of entries counted toward the size budget, where each entry counts for 1 regardless + // of size. + trackedCount uint64 + + // Advanced once per maintenance pass, and stamped onto an entry whenever a value is served from it + // or installed in it. Eviction prefers entries whose stamp is oldest. + epoch uint64 } // The result of a read from the underlying database. @@ -126,10 +111,16 @@ type cacheEntry struct { value []byte // The channel carrying the result of the read currently in flight for this key. Non-nil - // exactly while the status is statusScheduled: set by lookupLocked when it schedules a read, - // cleared by setTerminalStateLocked. Code running without the lock must use the channel + // exactly while the status is statusScheduled: set by LookupWLocked when it schedules a read, + // cleared by setTerminalEntryStateWLocked. Code running without the lock must use the channel // reference bound at scheduling time rather than reading this field. valueChan chan readResult + + // The epoch in which a reader last served a value from this entry. + lastRead atomic.Uint64 + + // This entry's contribution to trackedBytes, or zero while it holds no value. + size uint64 } // Tracks a key whose value is not yet available and must be waited on. @@ -143,7 +134,7 @@ type pendingRead struct { } // lookupOutcome is the result of classifying a single read under the lock: either an immediate -// terminal result, or a wait plan that resolve completes outside the lock. Classification never +// terminal result, or a wait plan that Resolve completes outside the lock. Classification never // fails — a cache that has seen a read failure is out of service and the shard refuses the read // before classifying it. type lookupOutcome struct { @@ -168,20 +159,19 @@ type lookupOutcome struct { needsSchedule bool } -// newReadCache creates a readCache sharing the given mutex (see the type doc for the locking +// NewReadCache creates a readCache sharing the given lock (see the type doc for the locking // contract). -func newReadCache( +func NewReadCache( ctx context.Context, + config *ViewManagerConfig, // The underlying key-value database. db types.KeyValueDB, // A work pool for asynchronous reads. readPool threading.Pool, - // The shard's mutex, shared with this cache. - lock *sync.Mutex, + // The shard's lock, borrowed by this cache. + lock *sync.RWMutex, // The maximum size of the cache, in bytes. maxSize uint64, - // The estimated bookkeeping overhead per entry, in bytes. - overheadPerEntry uint64, // Maps the context cancellation observed by a blocked read to the manager's shutdown error. shutdownError func() error, // Reports a failed DB read to the manager, which bricks and stops serving reads. @@ -189,37 +179,14 @@ func newReadCache( ) *readCache { return &readCache{ ctx: ctx, + config: config, db: db, readPool: readPool, lock: lock, shutdownError: shutdownError, reportReadFailure: reportReadFailure, - overheadPerEntry: overheadPerEntry, maxSize: maxSize, entries: make(map[string]*cacheEntry), - gcQueue: structures.NewLRUQueue(), - } -} - -// outOfServiceLocked returns a second-hand error if this cache has been taken out of service, or nil -// while it is healthy. The error is inherited from the earlier failure rather than produced by the -// caller's own operation. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) outOfServiceLocked() error { - if c.outOfServiceErr == nil { - return nil - } - return fmt.Errorf("shard is out of service: %w", c.outOfServiceErr) -} - -// takeOutOfServiceLocked records the failure that stops this cache from serving reads. The first -// failure wins; later ones are dropped so the reported cause is the original one. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) takeOutOfServiceLocked(err error) { - if c.outOfServiceErr == nil { - c.outOfServiceErr = err } } @@ -227,7 +194,7 @@ func (c *readCache) takeOutOfServiceLocked(err error) { // key is not found and reserving errors for actual failures (e.g. I/O errors). // // A nil value with found == true is impossible: per the types.KeyValueDB.Get contract, a found -// zero-length value is a non-nil empty slice. The read-completion paths (injectValue, resolve) +// zero-length value is a non-nil empty slice. The read-completion paths (injectValue, Resolve) // depend on this — they treat a nil value as not-found/deleted, so a backend that returned nil // for a stored empty value would silently turn that key into a tombstone. func (c *readCache) readFromDB(key []byte) (value []byte, found bool, err error) { @@ -241,12 +208,9 @@ func (c *readCache) readFromDB(key []byte) (value []byte, found bool, err error) return val, true, nil } -// setTerminalStateLocked records the entry's final status and value for this key and enrolls it in -// the LRU queue. A failed entry has no value to serve, so it is not enrolled. Eviction is left to -// the caller, since a bulk insert enforces the size budget once at the end rather than per entry. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (e *cacheEntry) setTerminalStateLocked(key []byte, status valueStatus, value []byte) { +// setTerminalEntryStateWLocked records the entry's final status and value for this key and counts it toward +// the size budget, unless the status is statusFailed. Eviction is left to the caller. +func (e *cacheEntry) setTerminalEntryStateWLocked(key []byte, status valueStatus, value []byte) { e.status = status e.value = value // Every waiter holds the channel reference it was scheduled with (see injectValue), so @@ -257,35 +221,67 @@ func (e *cacheEntry) setTerminalStateLocked(key []byte, status valueStatus, valu if status == statusFailed { return } - size := uint64(len(key)) + uint64(len(value)) + e.cache.overheadPerEntry - e.cache.gcQueue.Push(key, size) + e.cache.trackWLocked(e, uint64(len(key))+uint64(len(value))+e.cache.config.EstimatedOverheadPerEntry) } -// lookupLocked classifies a read of the given key and returns how to complete it: either an -// immediate terminal result, or a wait plan for resolve. Pure state transition: it never blocks, +// AttemptFastLookupRLocked answers a read of the given key when the cache already holds the answer, which +// is either a value or the knowledge that the key is absent. A found value is never nil, so found +// distinguishes the two. +// +// ok is false when the cache holds no answer yet, including when a read of the key is already in +// flight; the caller must then retry under the write lock via LookupWLocked. +func (c *readCache) AttemptFastLookupRLocked( + // The key to look up. + key []byte, + // If true, a cache hit marks the entry recently used. False is useful when an operation is + // performed multiple times in close succession on the same key, since the stamp has non-zero + // overhead and little benefit in that case. + updateLru bool, +) (value []byte, found bool, ok bool) { + entry := c.entryRLocked(key) + if entry == nil { + return nil, false, false + } + + switch entry.status { + case statusAvailable: + if updateLru { + entry.markRecentlyUsed(c.epoch) + } + return entry.value, true, true + case statusDeleted: + if updateLru { + entry.markRecentlyUsed(c.epoch) + } + return nil, false, true + default: + return nil, false, false + } +} + +// LookupWLocked classifies a read of the given key and returns how to complete it: either an +// immediate terminal result, or a wait plan for Resolve. Pure state transition: it never blocks, // and it performs the unknown -> scheduled transition under the lock, so a given read is // scheduled by exactly one caller. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) lookupLocked( +func (c *readCache) LookupWLocked( // The key to classify. key []byte, - // If true, a cache hit moves the entry to the back of the LRU queue. False is useful when an - // operation is performed multiple times in close succession on the same key, since the update - // has non-zero overhead and little benefit in that case. + // If true, a cache hit marks the entry recently used. False is useful when an operation is + // performed multiple times in close succession on the same key, since the stamp has non-zero + // overhead and little benefit in that case. updateLru bool, ) lookupOutcome { - entry := c.entryLocked(key, true) + entry := c.entryOrCreateWLocked(key) switch entry.status { case statusAvailable: if updateLru { - c.gcQueue.Touch(key) + entry.markRecentlyUsed(c.epoch) } return lookupOutcome{immediate: true, value: entry.value, found: true} case statusDeleted: if updateLru { - c.gcQueue.Touch(key) + entry.markRecentlyUsed(c.epoch) } return lookupOutcome{immediate: true} case statusScheduled: @@ -303,10 +299,9 @@ func (c *readCache) lookupLocked( } } -// resolve completes a read classified by lookupLocked. Must be called without the shared lock: -// it submits the DB read when this caller owns scheduling, and may block until the in-flight -// read completes. -func (c *readCache) resolve(key []byte, outcome lookupOutcome) ([]byte, bool, error) { +// ResolveUnlocked completes a read classified by LookupWLocked. It submits the DB read when this +// caller owns scheduling, and may block until the in-flight read completes. +func (c *readCache) ResolveUnlocked(key []byte, outcome lookupOutcome) ([]byte, bool, error) { if outcome.immediate { c.metrics.reportCacheHits(1) return outcome.value, outcome.found, nil @@ -320,7 +315,7 @@ func (c *readCache) resolve(key []byte, outcome lookupOutcome) ([]byte, bool, er ch := outcome.valueChan c.readPool.Submit(func() { value, _, readErr := c.readFromDB(key) - entry.injectValue(key, ch, readResult{value: value, err: readErr}) + entry.injectValueUnlocked(key, ch, readResult{value: value, err: readErr}) }) } @@ -338,14 +333,13 @@ func (c *readCache) resolve(key []byte, outcome lookupOutcome) ([]byte, bool, er return result.value, result.value != nil, nil } -// resolveBatch completes the pending reads of a batch classified via lookupLocked, writing found -// values into results. Must be called without the shared lock: it schedules the not-yet-scheduled -// reads and blocks until every pending read completes, then applies the terminal cache states -// asynchronously (bulkInjectValues). +// ResolveBatchUnlocked completes the pending reads of a batch classified via LookupWLocked, writing +// found values into results. It schedules the not-yet-scheduled reads and blocks until every pending +// read completes, then applies the terminal cache states asynchronously (bulkInjectValuesUnlocked). // // A non-nil return means the whole batch failed. The first read error is returned after the full // drain, unless the manager shuts down first. -func (c *readCache) resolveBatch(pending []pendingRead, results map[string][]byte) error { +func (c *readCache) ResolveBatchUnlocked(pending []pendingRead, results map[string][]byte) error { if len(pending) == 0 { return nil } @@ -393,15 +387,15 @@ func (c *readCache) resolveBatch(pending []pendingRead, results map[string][]byt } c.metrics.reportCacheMissLatency(time.Since(startTime)) - go c.bulkInjectValues(pending) + go c.bulkInjectValuesUnlocked(pending) return firstErr } // This method is called by the read scheduler when a value becomes available. ch is the channel -// bound at scheduling time (see resolve), which is the one every waiter on this read is blocked +// bound at scheduling time (see Resolve), which is the one every waiter on this read is blocked // on; e.valueChan may already have been detached by then. -func (e *cacheEntry) injectValue(key []byte, ch chan readResult, result readResult) { +func (e *cacheEntry) injectValueUnlocked(key []byte, ch chan readResult, result readResult) { c := e.cache c.lock.Lock() @@ -410,20 +404,20 @@ func (e *cacheEntry) injectValue(key []byte, ch chan readResult, result readResu // Terminal state so readers already waiting on this entry are not stranded. The manager // is bricked below, so the entry is never consulted again — the error reaches the waiter // over the bound channel, not from the entry. - e.setTerminalStateLocked(key, statusFailed, nil) + e.setTerminalEntryStateWLocked(key, statusFailed, nil) } else if result.value == nil { - e.setTerminalStateLocked(key, statusDeleted, nil) - c.evictLocked() + e.setTerminalEntryStateWLocked(key, statusDeleted, nil) + c.evictWLocked(c.hardCap()) } else { - e.setTerminalStateLocked(key, statusAvailable, result.value) - c.evictLocked() + e.setTerminalEntryStateWLocked(key, statusAvailable, result.value) + c.evictWLocked(c.hardCap()) } } // Take the cache out of service regardless of the entry's status: the DB read failed, which is // fatal whether or not this entry was still the one waiting on it. if result.err != nil { - c.takeOutOfServiceLocked(result.err) + c.TakeOutOfServiceWLocked(result.err) } c.lock.Unlock() @@ -438,7 +432,7 @@ func (e *cacheEntry) injectValue(key []byte, ch chan readResult, result readResu } // Applies deferred cache updates for a batch of reads under a single lock acquisition. -func (c *readCache) bulkInjectValues(reads []pendingRead) { +func (c *readCache) bulkInjectValuesUnlocked(reads []pendingRead) { c.lock.Lock() var failure error for i := range reads { @@ -458,36 +452,37 @@ func (c *readCache) bulkInjectValues(reads []pendingRead) { // Terminal state so readers already waiting on this entry are not stranded. The manager // is bricked below, so the entry is never consulted again — the error reaches the waiter // over the bound channel, not from the entry. - entry.setTerminalStateLocked(key, statusFailed, nil) + entry.setTerminalEntryStateWLocked(key, statusFailed, nil) } else if result.value == nil { - entry.setTerminalStateLocked(key, statusDeleted, nil) + entry.setTerminalEntryStateWLocked(key, statusDeleted, nil) } else { - entry.setTerminalStateLocked(key, statusAvailable, result.value) + entry.setTerminalEntryStateWLocked(key, statusAvailable, result.value) } } if failure != nil { - c.takeOutOfServiceLocked(failure) + c.TakeOutOfServiceWLocked(failure) } - c.evictLocked() + c.evictWLocked(c.hardCap()) c.lock.Unlock() - // The waiters for this batch were already released by resolveBatch, so there is nobody blocked + // The waiters for this batch were already released by ResolveBatch, so there is nobody blocked // on us while reportReadFailure acquires the manager's versionLock. if failure != nil { c.reportReadFailure(failure) } } -// Get a cache entry for a given key. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) entryLocked(key []byte, createIfMissing bool) *cacheEntry { +// entryRLocked returns the cache entry for a given key, or nil if the cache holds none. +func (c *readCache) entryRLocked(key []byte) *cacheEntry { + return c.entries[string(key)] +} + +// entryOrCreateWLocked returns the cache entry for a given key, creating one whose value is not yet +// known if the cache holds none. Never returns nil. +func (c *readCache) entryOrCreateWLocked(key []byte) *cacheEntry { if entry, ok := c.entries[string(key)]; ok { return entry } - if !createIfMissing { - return nil - } entry := &cacheEntry{ cache: c, status: statusUnknown, @@ -496,59 +491,138 @@ func (c *readCache) entryLocked(key []byte, createIfMissing bool) *cacheEntry { return entry } -// putRetiredLocked installs data retired out of the shard's MVCC layer. A nil value marks the +// PutRetiredWLocked installs data retired out of the shard's MVCC layer. A nil value marks the // key as known-deleted (the manager-wide tombstone convention); any other value is cached as // available. Inserts everything, then evicts overflow once at the end. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) putRetiredLocked(data map[string][]byte) { +func (c *readCache) PutRetiredWLocked(data map[string][]byte) { for k, v := range data { if v == nil { - c.deleteRetiredLocked([]byte(k)) + c.deleteRetiredWLocked([]byte(k)) } else { - c.setRetiredLocked([]byte(k), v) + c.setRetiredWLocked([]byte(k), v) } } // These insertions may have caused the cache to exceed its size budget, do necessary - // evictions. setRetiredLocked does not evict on its own, so this is the enforcement point + // evictions. setRetiredWLocked does not evict on its own, so this is the enforcement point // for the bulk insert above. - c.evictLocked() + c.evictWLocked(c.hardCap()) } // Set a retired value. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) setRetiredLocked(key []byte, value []byte) { - entry := c.entryLocked(key, true) - entry.setTerminalStateLocked(key, statusAvailable, value) +func (c *readCache) setRetiredWLocked(key []byte, value []byte) { + entry := c.entryOrCreateWLocked(key) + entry.setTerminalEntryStateWLocked(key, statusAvailable, value) } // Delete a retired value. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) deleteRetiredLocked(key []byte) { - entry := c.entryLocked(key, false) +func (c *readCache) deleteRetiredWLocked(key []byte) { + entry := c.entryRLocked(key) if entry == nil { // Key is not in the cache, so nothing to do. return } - entry.setTerminalStateLocked(key, statusDeleted, nil) + entry.setTerminalEntryStateWLocked(key, statusDeleted, nil) } -// Evicts least recently used entries until the cache is within its size budget. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) evictLocked() { - for c.gcQueue.GetTotalSize() > c.maxSize { - next := c.gcQueue.PopLeastRecentlyUsed() - delete(c.entries, next) +// markRecentlyUsed records that a reader served a value from this entry in the given epoch, making it +// a later candidate for eviction. +func (e *cacheEntry) markRecentlyUsed(epoch uint64) { + if e.lastRead.Load() != epoch { + // The load guards the store to keep this entry's cache line in shared state on a repeat read: + // concurrent readers need that same line for the value, and storing would take it exclusive. + e.lastRead.Store(epoch) } } -// sizeInfoLocked returns the current size (bytes) and entry count. -// -// The Locked postfix indicates that the caller must hold the shared lock. -func (c *readCache) sizeInfoLocked() (bytes uint64, entries uint64) { - return c.gcQueue.GetTotalSize(), c.gcQueue.GetCount() +// trackWLocked records an entry's contribution to the size budget, replacing whatever it contributed +// before, and stamps the entry as read in the current epoch. +func (c *readCache) trackWLocked(entry *cacheEntry, size uint64) { + if entry.size == 0 { + c.trackedCount++ + } + c.trackedBytes -= entry.size + c.trackedBytes += size + entry.size = size + + // A newly tracked entry counts as read now. Without this an entry inserted just before a sweep + // looks infinitely old and is evicted immediately, which would throw away the read that fetched it. + entry.lastRead.Store(c.epoch) +} + +// untrackWLocked removes an entry from the size budget and from the cache. +func (c *readCache) untrackWLocked(key string, entry *cacheEntry) { + c.trackedBytes -= entry.size + c.trackedCount-- + entry.size = 0 + delete(c.entries, key) +} + +// evictWLocked evicts entries until the cache is within the given budget, choosing each victim as the +// oldest of a small sample. Only entries counted toward the budget are eligible; entries that are not +// still count against the sample. Falls short of the budget when the sample holds no eligible entry. +func (c *readCache) evictWLocked(budget uint64) { + for c.trackedBytes > budget { + var victimKey string + var victim *cacheEntry + oldest := uint64(math.MaxUint64) + + var visited uint64 + for key, entry := range c.entries { + visited++ + // An entry holding no value has nothing to reclaim and no stamp worth comparing, but it has + // still consumed a step of the walk. + if entry.size > 0 { + if stamp := entry.lastRead.Load(); stamp <= oldest { + oldest, victimKey, victim = stamp, key, entry + } + } + if visited == c.config.EvictionSampleSize { + break + } + } + + if victim == nil { + // The walk found nothing to evict, either because the sample happened to hold no values or + // because the accounting disagrees with the map. Stopping is the safe response either way: + // running over budget costs memory, whereas looping here would spin while holding the shard + // lock. The next maintenance pass samples a different part of the map and makes progress. + return + } + c.untrackWLocked(victimKey, victim) + } +} + +// SizeInfoRLocked returns the current size (bytes) and entry count. +func (c *readCache) SizeInfoRLocked() (bytes uint64, entries uint64) { + return c.trackedBytes, c.trackedCount +} + +// hardCap is the ceiling that insertions enforce inline. +func (c *readCache) hardCap() uint64 { + return c.maxSize + c.maxSize/c.config.EvictionSlackDivisor +} + +// MaintainWLocked advances the epoch and brings the cache back within its size budget. +func (c *readCache) MaintainWLocked() { + c.epoch++ + c.evictWLocked(c.maxSize) +} + +// ErrIfOutOfServiceRLocked returns a second-hand error if this cache has been taken out of service, or nil +// while it is healthy. The error is inherited from the earlier failure rather than produced by the +// caller's own operation. +func (c *readCache) ErrIfOutOfServiceRLocked() error { + if c.outOfServiceErr == nil { + return nil + } + return fmt.Errorf("shard is out of service: %w", c.outOfServiceErr) +} + +// TakeOutOfServiceWLocked records the failure that stops this cache from serving reads. The first +// failure wins; later ones are dropped so the reported cause is the original one. +func (c *readCache) TakeOutOfServiceWLocked(err error) { + if c.outOfServiceErr == nil { + c.outOfServiceErr = err + } } diff --git a/sei-db/db_engine/view/read_cache_test.go b/sei-db/db_engine/view/read_cache_test.go index cad969ec72..5208d966c4 100644 --- a/sei-db/db_engine/view/read_cache_test.go +++ b/sei-db/db_engine/view/read_cache_test.go @@ -1,6 +1,7 @@ package view import ( + "fmt" "math/rand" "testing" @@ -30,7 +31,7 @@ func TestResolveDeliversThroughBoundChannel(t *testing.T) { // // AdHocPool.Submit spawns a goroutine, so a fuzz loop that went through resolve() end to end would // always block until the read had already completed and could never observe a statusScheduled -// entry — there would be no window to race a retire against. Driving lookupLocked/injectValue +// entry — there would be no window to race a retire against. Driving LookupWLocked/injectValue // directly instead makes that interleaving deterministic and reproducible without any goroutines, // gates, or timing dependency. func TestValueChannelAttachedOnlyWhileScheduled(t *testing.T) { @@ -50,11 +51,24 @@ func TestValueChannelAttachedOnlyWhileScheduled(t *testing.T) { checkInvariant := func() { shard.lock.Lock() defer shard.lock.Unlock() + + var bytes uint64 + var count uint64 for key, entry := range shard.cache.entries { require.Equal(t, entry.status == statusScheduled, entry.valueChan != nil, "entry %q has status %v with valueChan != nil == %v", key, entry.status, entry.valueChan != nil) + bytes += entry.size + if entry.size > 0 { + count++ + } } + + // The size budget is tracked by the cache rather than derived from the entry map, so every + // path that gives an entry a value, replaces one, or drops an entry has to keep the two in + // step. A retire landing on a live value and an eviction are both such paths. + require.Equal(t, bytes, shard.cache.trackedBytes, "trackedBytes disagrees with the entries") + require.Equal(t, count, shard.cache.trackedCount, "trackedCount disagrees with the entries") } racedCompletions := 0 @@ -65,7 +79,7 @@ func TestValueChannelAttachedOnlyWhileScheduled(t *testing.T) { shard.lock.Lock() switch rng.Intn(5) { case 0: // start a read - outcome := shard.cache.lookupLocked([]byte(key), true) + outcome := shard.cache.LookupWLocked([]byte(key), true) if outcome.needsSchedule { pending[key] = outcome } @@ -75,18 +89,18 @@ func TestValueChannelAttachedOnlyWhileScheduled(t *testing.T) { entry := outcome.entry raced := entry.status != statusScheduled shard.lock.Unlock() - entry.injectValue([]byte(key), outcome.valueChan, readResult{value: randomValue()}) + entry.injectValueUnlocked([]byte(key), outcome.valueChan, readResult{value: randomValue()}) shard.lock.Lock() if raced { racedCompletions++ } } case 2: // retire: set - shard.cache.setRetiredLocked([]byte(key), randomValue()) + shard.cache.setRetiredWLocked([]byte(key), randomValue()) case 3: // retire: delete - shard.cache.deleteRetiredLocked([]byte(key)) + shard.cache.deleteRetiredWLocked([]byte(key)) case 4: // evict - shard.cache.evictLocked() + shard.cache.evictWLocked(shard.cache.hardCap()) } shard.lock.Unlock() @@ -96,3 +110,38 @@ func TestValueChannelAttachedOnlyWhileScheduled(t *testing.T) { require.Greater(t, racedCompletions, 0, "fuzz run never exercised a retire landing on an in-flight read; the seed/op mix needs adjusting") } + +// TestMaintenanceEvictsBackToBudget checks that the once-per-block maintenance pass brings a cache +// that has been pushed past its budget back within it. +// +// Which keys survive is deliberately not asserted: eviction samples the entry map, so the victims +// depend on map iteration order. Correctness does not, since an evicted key is read from the backing +// DB again. +func TestMaintenanceEvictsBackToBudget(t *testing.T) { + const maxSize = 1024 + const entrySize = 16 // 8-byte key + 8-byte value, with the test config's zero per-entry overhead + + shard := newTestShard(t, maxSize, newTestDB(nil)) + + // Twice the budget, inserted as a retirement so every entry lands in a terminal state. + retired := make(map[string][]byte) + for i := 0; i < 2*maxSize/entrySize; i++ { + retired[fmt.Sprintf("key%05d", i)] = []byte(fmt.Sprintf("val%05d", i)) + } + shard.lock.Lock() + shard.cache.PutRetiredWLocked(retired) + overBudget, _ := shard.cache.SizeInfoRLocked() + hardCap := shard.cache.hardCap() + shard.lock.Unlock() + + // The insert path enforces only the hard cap, so the cache is expected to sit above its budget + // until maintenance runs; a test that started under budget would prove nothing. + require.Greater(t, overBudget, uint64(maxSize)) + require.LessOrEqual(t, overBudget, hardCap) + + shard.Commit() + + bytes, entries := shard.GetSizeInfo() + require.LessOrEqual(t, bytes, uint64(maxSize)) + require.Equal(t, bytes, entries*entrySize) +} diff --git a/sei-db/db_engine/view/shard.go b/sei-db/db_engine/view/shard.go index fdfbc7b7d1..5f98c27d67 100644 --- a/sei-db/db_engine/view/shard.go +++ b/sei-db/db_engine/view/shard.go @@ -23,12 +23,12 @@ import ( // touches a shard, which is illegal. // - The database crashed. Database failures are fatal and are never recovered from, so every shard // goes out of service, not just the one that saw the failure. +// +// Method postfixes state the lock contract: RLocked and WLocked require the caller to hold the read or +// write lock, Unlocked requires the caller to hold neither, and a bare name has no lock dependency. type shard struct { - // A lock to protect the shard's data. Shared with the read cache (see the cache field). - // - // TODO: this is a single exclusive lock. If it becomes a contention bottleneck, consider an RW - // lock — see the conversion strategy at the top of read_cache.go. - lock sync.Mutex + // A lock to protect the shard's data. Also used by the read cache (see the cache field). + lock sync.RWMutex // Data at various versions. This is for data that has not yet been flushed down into the DB. versionedData map[string] /* key */ *structures.Deque[versionedValue] /* values at various versions */ @@ -39,7 +39,7 @@ type shard struct { versionDiffs map[uint64] /* version */ map[string] /* key */ []byte /* value */ // The read-through DB cache backing this shard. A passive component sharing this shard's - // lock: its xxxLocked methods require the lock held, while its resolve methods and background + // lock: its RLocked and WLocked methods require the lock held, while its Resolve methods and background // read-completion paths manage their own synchronization. The cache never calls outward while // holding the lock — it never calls into the shard at all, and its one call into the manager // (reportReadFailure, which acquires versionLock) is made only after releasing the lock — so @@ -58,8 +58,6 @@ type shard struct { // The number of iterators currently reading this shard. Close reports a non-zero count as a // leaked iterator, since reading one after the database has closed is undefined behaviour (see // ViewManager.Close). - // - // Guarded by lock. openIterators uint64 } @@ -112,63 +110,88 @@ func NewShard( currentVersion: 1, // important: versions start at 1, not 0, to allow (version - 1) without underflow oldestVersion: 1, } - s.cache = newReadCache( - ctx, db, readPool, &s.lock, maxSize, config.EstimatedOverheadPerEntry, shutdownError, reportReadFailure) + s.cache = NewReadCache(ctx, config, db, readPool, &s.lock, maxSize, shutdownError, reportReadFailure) return s, nil } -// takeOutOfService stops this shard from serving reads and accepting writes, reporting err as the -// cause. Called on every shard when the manager shuts down, so a failure anywhere stops every shard. -func (s *shard) takeOutOfService(err error) { - s.lock.Lock() - s.cache.takeOutOfServiceLocked(err) - s.lock.Unlock() -} - // Get returns the value for the given key, or (nil, false, nil) if not found at the given version. func (s *shard) Get( // The key to get. key []byte, // The version of the data to get. version uint64, - // If true, the LRU queue will be updated. If false, the LRU queue will not be updated. - // Useful for when an operation is performed multiple times in close succession on the same key, - // since it requires non-zero overhead to do so with little benefit. + // If true, the entry's recency is recorded. If false, it is not. Useful for when an operation is + // performed multiple times in close succession on the same key, since it requires non-zero + // overhead to do so with little benefit. updateLru bool, ) ([]byte, bool, error) { + if value, found, done, err := s.attemptFastGetUnlocked(key, version, updateLru); done { + return value, found, err + } + + // Not resolvable without mutating: classify against the DB read-cache under the write lock, + // then complete the read (which may schedule a DB read and block) outside it. Redone from scratch + // because the lock was released in between, so another reader may have scheduled this key. s.lock.Lock() // Checked ahead of the versioned data so that a shard taken out of service refuses every read, // not just those that would have reached the DB. - if err := s.cache.outOfServiceLocked(); err != nil { + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { s.lock.Unlock() return nil, false, err } - if err := s.validateVersionLocked(version); err != nil { + if err := s.validateVersionRLocked(version); err != nil { s.lock.Unlock() return nil, false, err } // First, check to see if we have this value in the versioned data map. - if value, found := s.lookupVersionedLocked(string(key), version); found { + if value, found := s.lookupVersionedRLocked(string(key), version); found { s.lock.Unlock() s.metrics.reportCacheHits(1) return value, value != nil, nil } - // Not in the versioned data map: classify against the DB read-cache under the same lock - // grab, then complete the read (which may schedule a DB read and block) outside the lock. - outcome := s.cache.lookupLocked(key, updateLru) + outcome := s.cache.LookupWLocked(key, updateLru) s.lock.Unlock() - return s.cache.resolve(key, outcome) + return s.cache.ResolveUnlocked(key, outcome) } -// validateVersionLocked checks that the given version is within the valid range. -// -// The Locked postfix indicates that the caller must hold the shard lock. -func (s *shard) validateVersionLocked(version uint64) error { +// attemptFastGetUnlocked attempts a read while holding only the read lock, reporting done when it +// succeeded. A non-nil err always comes with done. A read it could not resolve without mutating is +// left to the caller to redo under the write lock. +func (s *shard) attemptFastGetUnlocked( + key []byte, + version uint64, + updateLru bool, +) (value []byte, found bool, done bool, err error) { + s.lock.RLock() + defer s.lock.RUnlock() + + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { + return nil, false, true, err + } + if err := s.validateVersionRLocked(version); err != nil { + return nil, false, true, err + } + + if value, found := s.lookupVersionedRLocked(string(key), version); found { + s.metrics.reportCacheHits(1) + return value, value != nil, true, nil + } + + value, found, ok := s.cache.AttemptFastLookupRLocked(key, updateLru) + if !ok { + return nil, false, false, nil + } + s.metrics.reportCacheHits(1) + return value, found, true, nil +} + +// validateVersionRLocked checks that the given version is within the valid range. +func (s *shard) validateVersionRLocked(version uint64) error { if version < s.oldestVersion { return fmt.Errorf("version (%d) is less than the oldest version (%d)", version, s.oldestVersion) } @@ -178,12 +201,10 @@ func (s *shard) validateVersionLocked(version uint64) error { return nil } -// lookupVersionedLocked checks versioned data for a key at the given version. +// lookupVersionedRLocked checks versioned data for a key at the given version. // Returns (value, true) if found in versioned data, (nil, false) if the read cache should be // consulted. -// -// The Locked postfix indicates that the caller must hold the shard lock. -func (s *shard) lookupVersionedLocked(key string, version uint64) ([]byte, bool) { +func (s *shard) lookupVersionedRLocked(key string, version uint64) ([]byte, bool) { deque, ok := s.versionedData[key] if !ok { return nil, false @@ -209,26 +230,56 @@ func (s *shard) lookupVersionedLocked(key string, version uint64) ([]byte, bool) // error fails the whole call and returns a nil map. func (s *shard) BatchGet(keys [][]byte, version uint64) (map[string][]byte, error) { results := make(map[string][]byte, len(keys)) - pending := make([]pendingRead, 0, len(keys)) - var hits int64 - s.lock.Lock() + unresolved, hits, err := s.attemptFastBatchGetUnlocked(keys, results, version) + if err != nil { + return nil, err + } + + var pending []pendingRead + if len(unresolved) > 0 { + var remainingHits int64 + pending, remainingHits, err = s.batchGetRemainingUnlocked(keys, unresolved, results, version) + if err != nil { + return nil, err + } + hits += remainingHits + } + + if hits > 0 { + s.metrics.reportCacheHits(hits) + } + + if err := s.cache.ResolveBatchUnlocked(pending, results); err != nil { + // DB errors are fatal; fail the whole batch. + return nil, err + } + return results, nil +} + +// attemptFastBatchGetUnlocked resolves the keys it can while holding the read lock, writing found +// values into results and returning the positions in keys of those it could not resolve. +func (s *shard) attemptFastBatchGetUnlocked( + keys [][]byte, + results map[string][]byte, + version uint64, +) (unresolved []int, hits int64, err error) { + s.lock.RLock() + defer s.lock.RUnlock() // Checked ahead of the versioned data so that a shard taken out of service refuses every read, // not just those that would have reached the DB. - if err := s.cache.outOfServiceLocked(); err != nil { - s.lock.Unlock() - return nil, err + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { + return nil, 0, err } - if err := s.validateVersionLocked(version); err != nil { - s.lock.Unlock() - return nil, err + if err := s.validateVersionRLocked(version); err != nil { + return nil, 0, err } - for _, key := range keys { + for i, key := range keys { keyStr := string(key) - if value, found := s.lookupVersionedLocked(keyStr, version); found { + if value, found := s.lookupVersionedRLocked(keyStr, version); found { // found includes tombstones (nil value); only non-nil values are real hits to return. if value != nil { results[keyStr] = value @@ -237,10 +288,57 @@ func (s *shard) BatchGet(keys [][]byte, version uint64) (map[string][]byte, erro continue } - // The batch path never touches the LRU queue on hits, hence updateLru=false. - outcome := s.cache.lookupLocked(key, false) - if outcome.immediate { + // The batch path never records recency on hits, hence updateLru=false. + value, found, ok := s.cache.AttemptFastLookupRLocked(key, false) + if ok { // Resolved from cache. A not-found (deleted) key counts as a hit but is not a result. + if found { + results[keyStr] = value + } + hits++ + continue + } + unresolved = append(unresolved, i) + } + return unresolved, hits, nil +} + +// batchGetRemainingUnlocked classifies the keys at the given positions in keys, which are those the +// fast pass could not resolve, creating entries and scheduling DB reads as needed. +func (s *shard) batchGetRemainingUnlocked( + keys [][]byte, + indices []int, + results map[string][]byte, + version uint64, +) (pending []pendingRead, hits int64, err error) { + pending = make([]pendingRead, 0, len(indices)) + + s.lock.Lock() + defer s.lock.Unlock() + + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { + return nil, 0, err + } + + if err := s.validateVersionRLocked(version); err != nil { + return nil, 0, err + } + + // Redone from scratch rather than carried over from the fast pass, because the lock was released + // in between and another reader may have scheduled or completed any of these keys. + for _, i := range indices { + key := keys[i] + keyStr := string(key) + if value, found := s.lookupVersionedRLocked(keyStr, version); found { + if value != nil { + results[keyStr] = value + } + hits++ + continue + } + + outcome := s.cache.LookupWLocked(key, false) + if outcome.immediate { if outcome.found { results[keyStr] = outcome.value } @@ -254,37 +352,27 @@ func (s *shard) BatchGet(keys [][]byte, version uint64) (map[string][]byte, erro needsSchedule: outcome.needsSchedule, }) } - s.lock.Unlock() - - if hits > 0 { - s.metrics.reportCacheHits(hits) - } - - if err := s.cache.resolveBatch(pending, results); err != nil { - // DB errors are fatal; fail the whole batch. - return nil, err - } - return results, nil + return pending, hits, nil } -// getSizeInfo returns the current cache size (bytes) and entry count under the shard lock. -func (s *shard) getSizeInfo() (bytes uint64, entries uint64) { - s.lock.Lock() - defer s.lock.Unlock() - return s.cache.sizeInfoLocked() +// GetSizeInfo returns the current cache size (bytes) and entry count under the read lock. +func (s *shard) GetSizeInfo() (bytes uint64, entries uint64) { + s.lock.RLock() + defer s.lock.RUnlock() + return s.cache.SizeInfoRLocked() } -// iteratorOpened records that an iterator is reading this shard. Balanced by exactly one -// iteratorClosed. -func (s *shard) iteratorOpened() { +// IteratorOpened records that an iterator is reading this shard. Balanced by exactly one +// IteratorClosed. +func (s *shard) IteratorOpened() { s.lock.Lock() s.openIterators++ s.lock.Unlock() } -// iteratorClosed records that an iterator reading this shard has been closed. Closing more than were +// IteratorClosed records that an iterator reading this shard has been closed. Closing more than were // opened is refused rather than wrapping the count, which would make the leak report at Close useless. -func (s *shard) iteratorClosed() error { +func (s *shard) IteratorClosed() error { s.lock.Lock() defer s.lock.Unlock() @@ -303,17 +391,15 @@ func (s *shard) Set(key []byte, value []byte) error { s.lock.Lock() defer s.lock.Unlock() - if err := s.cache.outOfServiceLocked(); err != nil { + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { return err } - s.setLocked(key, value) + s.setWLocked(key, value) return nil } -// setLocked writes a value to the versioned data structures at the current version. -// -// The Locked postfix indicates that the caller must hold the shard lock. -func (s *shard) setLocked(key []byte, value []byte) { +// setWLocked writes a value to the versioned data structures at the current version. +func (s *shard) setWLocked(key []byte, value []byte) { keyStr := string(key) s.versionDiffs[s.currentVersion][keyStr] = value @@ -337,15 +423,15 @@ func (s *shard) BatchSet(entries []*proto.KVPair) error { defer s.lock.Unlock() // Checked once for the whole batch rather than per key: it cannot change while we hold the lock. - if err := s.cache.outOfServiceLocked(); err != nil { + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { return err } for i := range entries { if entries[i].Delete { // A delete is stored as a nil-valued (tombstone) entry at the current version. - s.setLocked(entries[i].Key, nil) + s.setWLocked(entries[i].Key, nil) } else { - s.setLocked(entries[i].Key, entries[i].Value) + s.setWLocked(entries[i].Key, entries[i].Value) } } return nil @@ -356,7 +442,8 @@ func (s *shard) Delete(key []byte) error { return s.Set(key, nil) } -// Commit seals the current version; all future updates will be applied to the next version. +// Commit seals the current version; all future updates will be applied to the next version. It also +// runs the read cache's once-per-block maintenance. // The value returned is the new version number (for sanity checking). func (s *shard) Commit() uint64 { s.lock.Lock() @@ -366,6 +453,10 @@ func (s *shard) Commit() uint64 { s.versionDiffs[newVersion] = make(map[string][]byte) + // Sealing a version is the once-per-block moment the read cache does its eviction, so that no read + // has to pay for it. + s.cache.MaintainWLocked() + s.lock.Unlock() return newVersion @@ -385,8 +476,11 @@ func (s *shard) GetDiffsForVersions( firstVersion, lastVersion) } - s.lock.Lock() - defer s.lock.Unlock() + // A read lock suffices, and it matters: sort jobs for different versions call this concurrently. + // Nothing here mutates the shard, and the maps handed back are frozen — only versionDiffs at the + // current version is ever written to, so a version stops changing the moment it is no longer current. + s.lock.RLock() + defer s.lock.RUnlock() if firstVersion < s.oldestVersion { return nil, fmt.Errorf("firstVersion (%d) must be greater than or equal to the oldest version (%d)", @@ -404,19 +498,19 @@ func (s *shard) GetDiffsForVersions( return diffs, nil } -// materializeCurrentOverrides returns the in-memory overrides in this shard at the current version +// MaterializeCurrentOverrides returns the in-memory overrides in this shard at the current version // whose keys fall within [lowerBound, upperBound). A nil bound is unbounded on that side. The result // is unsorted. // // Because the target is always the current version, each key resolves to the back of its deque — -// no version scan is needed, unlike lookupVersionedLocked, which serves reads at older versions. -func (s *shard) materializeCurrentOverrides(lowerBound []byte, upperBound []byte) ([]kvPair, error) { - s.lock.Lock() - defer s.lock.Unlock() +// no version scan is needed, unlike lookupVersionedRLocked, which serves reads at older versions. +func (s *shard) MaterializeCurrentOverrides(lowerBound []byte, upperBound []byte) ([]kvPair, error) { + s.lock.RLock() + defer s.lock.RUnlock() // Same reason the read paths check it: a shard taken out of service cannot vouch for its data, // and an iterator is just a bulk read. - if err := s.cache.outOfServiceLocked(); err != nil { + if err := s.cache.ErrIfOutOfServiceRLocked(); err != nil { return nil, err } @@ -503,10 +597,18 @@ func (s *shard) DropVersions( // Push the combined data down into the read cache, still under the same lock grab, so // readers never observe an intermediate state between the deque cleanup and the cache // insert. - s.cache.putRetiredLocked(combinedData) + s.cache.PutRetiredWLocked(combinedData) // Update the oldest version. s.oldestVersion = lastVersion return nil } + +// TakeOutOfService stops this shard from serving reads and accepting writes, reporting err as the +// cause. Called on every shard when the manager shuts down, so a failure anywhere stops every shard. +func (s *shard) TakeOutOfService(err error) { + s.lock.Lock() + s.cache.TakeOutOfServiceWLocked(err) + s.lock.Unlock() +} diff --git a/sei-db/db_engine/view/view_iterator_test.go b/sei-db/db_engine/view/view_iterator_test.go index a746a63382..2b05bae238 100644 --- a/sei-db/db_engine/view/view_iterator_test.go +++ b/sei-db/db_engine/view/view_iterator_test.go @@ -174,7 +174,7 @@ func TestIteratorClosedRefusesUnderflow(t *testing.T) { shard := manager.(*viewManager).shards[0] require.Equal(t, uint64(0), openIteratorCount(manager)) - require.Error(t, shard.iteratorClosed(), "closing past zero must be refused") + require.Error(t, shard.IteratorClosed(), "closing past zero must be refused") require.Equal(t, uint64(0), openIteratorCount(manager), "a refused close must not wrap the count") } diff --git a/sei-db/db_engine/view/view_manager.go b/sei-db/db_engine/view/view_manager.go index 9d0fd8b76f..dd0f595ae1 100644 --- a/sei-db/db_engine/view/view_manager.go +++ b/sei-db/db_engine/view/view_manager.go @@ -177,9 +177,9 @@ type View interface { Get( // The entry to fetch. key []byte, - // If true, the LRU queue will be updated. If false, the LRU queue will not be updated. - // Useful for when an operation is performed multiple times in close succession on the same key, - // since it requires non-zero overhead to do so with little benefit. + // If true, the entry's recency is recorded, making it a later candidate for cache eviction. + // Useful to set false when an operation is performed multiple times in close succession on the + // same key, since it requires non-zero overhead to do so with little benefit. updateLru bool, ) ([]byte, bool, error) diff --git a/sei-db/db_engine/view/view_manager_config.go b/sei-db/db_engine/view/view_manager_config.go index 9c20d9bc96..7cef4dc39a 100644 --- a/sei-db/db_engine/view/view_manager_config.go +++ b/sei-db/db_engine/view/view_manager_config.go @@ -61,6 +61,14 @@ type ViewManagerConfig struct { // lost (but the DB is not corrupted), and crash durability is instead provided by an upstream // fsync'd WAL / block replay. Set true for deployments that want per-flush durability regardless. FlushSync bool + + // How far over its size budget a shard's read cache may run between maintenance passes, expressed + // as a divisor of that budget: a value of N permits an overshoot of budget/N. + EvictionSlackDivisor uint64 + + // The number of entries a shard's read cache considers when choosing an eviction victim: it evicts + // the least recently used of a sample this size. + EvictionSampleSize uint64 } // Default configuration for a production view manager. name and reservedPrefix are arguments @@ -78,6 +86,8 @@ func DefaultViewManagerConfig(name string, reservedPrefix string) *ViewManagerCo TargetBytesPerFlush: unit.MB * 4, ReservedPrefix: reservedPrefix, FlushSync: false, + EvictionSlackDivisor: 16, + EvictionSampleSize: 8, } } @@ -118,6 +128,14 @@ func (c *ViewManagerConfig) Validate() error { if c.TargetBytesPerFlush == 0 { return fmt.Errorf("TargetBytesPerFlush must be greater than 0") } + // Zero would divide by zero in the cache's hard-cap calculation. + if c.EvictionSlackDivisor == 0 { + return fmt.Errorf("EvictionSlackDivisor must be greater than 0") + } + // Zero would leave eviction unable to find a victim, so the cache would grow without bound. + if c.EvictionSampleSize == 0 { + return fmt.Errorf("EvictionSampleSize must be greater than 0") + } if c.ReservedPrefix == "" { return fmt.Errorf("ReservedPrefix must be non-empty") } diff --git a/sei-db/db_engine/view/view_manager_impl.go b/sei-db/db_engine/view/view_manager_impl.go index 2051380af1..0e4fedd0a7 100644 --- a/sei-db/db_engine/view/view_manager_impl.go +++ b/sei-db/db_engine/view/view_manager_impl.go @@ -229,7 +229,7 @@ func NewViewManager( func (c *viewManager) getCacheSizeInfo() (bytes uint64, entries uint64) { for _, s := range c.shards { - b, e := s.getSizeInfo() + b, e := s.GetSizeInfo() bytes += b entries += e } @@ -369,9 +369,9 @@ func (c *viewManager) Commit() (View, error) { // bricked) has no lifecycle runner left to flush what a new version would stage, so sealing one // would discard it silently. for i, s := range c.shards { - s.lock.Lock() - err := s.cache.outOfServiceLocked() - s.lock.Unlock() + s.lock.RLock() + err := s.cache.ErrIfOutOfServiceRLocked() + s.lock.RUnlock() if err != nil { return nil, fmt.Errorf("cannot create view, shard %d: %w", i, err) } @@ -658,7 +658,7 @@ func (c *viewManager) Iterator(opts *types.IterOptions) (dbm.Iterator, error) { // that moved data out of versionedData and into the DB between the two steps would drop those // keys entirely if the DB view were taken first. In this order the same race can only yield a // key twice, which the merge resolves in favor of the override. - overrides, err := c.materializeCurrentOverrides(opts) + overrides, err := c.MaterializeCurrentOverrides(opts) if err != nil { return nil, fmt.Errorf("failed to materialize current overrides: %w", err) } @@ -682,7 +682,7 @@ func (c *viewManager) Iterator(opts *types.IterOptions) (dbm.Iterator, error) { // Register the iterator only now that construction has fully succeeded, so a failed construction // cannot leave a phantom entry behind. for _, s := range c.shards { - s.iteratorOpened() + s.IteratorOpened() } return &trackedIterator{Iterator: iter, manager: c}, nil } @@ -700,7 +700,7 @@ func (w *trackedIterator) Close() error { w.closeOnce.Do(func() { errs := make([]error, 0, len(w.manager.shards)+1) for _, s := range w.manager.shards { - errs = append(errs, s.iteratorClosed()) + errs = append(errs, s.IteratorClosed()) } errs = append(errs, w.Iterator.Close()) err = errors.Join(errs...) @@ -708,12 +708,12 @@ func (w *trackedIterator) Close() error { return err } -// materializeCurrentOverrides gathers the in-memory overrides at the current version from every +// MaterializeCurrentOverrides gathers the in-memory overrides at the current version from every // shard and returns them sorted ascending by key. Each shard is responsible for its own locking; // here we just stitch the results together, and the sort runs without any shard lock held. // The overrides are sorted into iteration order — ascending, or descending when reverse is set — so // the merge in viewIterator can walk them and the DB iterator in lockstep. -func (c *viewManager) materializeCurrentOverrides(opts *types.IterOptions) ([]kvPair, error) { +func (c *viewManager) MaterializeCurrentOverrides(opts *types.IterOptions) ([]kvPair, error) { var lowerBound, upperBound []byte reverse := false if opts != nil { @@ -722,7 +722,7 @@ func (c *viewManager) materializeCurrentOverrides(opts *types.IterOptions) ([]kv var all []kvPair for i, s := range c.shards { - shardOverrides, err := s.materializeCurrentOverrides(lowerBound, upperBound) + shardOverrides, err := s.MaterializeCurrentOverrides(lowerBound, upperBound) if err != nil { return nil, fmt.Errorf("shard %d: %w", i, err) } @@ -807,7 +807,7 @@ func (c *viewManager) brickLocked(err error) { // established order (see Commit), and nothing acquires versionLock while holding a shard lock // (see the cache field on shard). for _, s := range c.shards { - s.takeOutOfService(err) + s.TakeOutOfService(err) } } @@ -1142,7 +1142,7 @@ func (c *viewManager) closeInternal() error { // write accepted from here on could never be flushed. First failure wins inside the shard, so a // brick that already ran keeps reporting its own cause rather than ErrViewManagerClosed. for _, s := range c.shards { - s.takeOutOfService(ErrViewManagerClosed) + s.TakeOutOfService(ErrViewManagerClosed) } c.versionLock.Unlock() c.lifecycleBackpressureCond.Broadcast() @@ -1177,9 +1177,9 @@ func (c *viewManager) closeInternal() error { // is read under its own lock. The manager always has at least one shard (the config requires it). func (c *viewManager) assertNoLeakedIterators() error { s := c.shards[0] - s.lock.Lock() + s.lock.RLock() open := s.openIterators - s.lock.Unlock() + s.lock.RUnlock() if open == 0 { return nil From 91b5998a92fa4670c5806ec4442e6b214d57a5b4 Mon Sep 17 00:00:00 2001 From: Cody Littley Date: Fri, 11 Sep 2026 08:33:35 -0500 Subject: [PATCH 2/2] made suggested changes --- sei-db/db_engine/view/read_cache.go | 59 ++++++++++++-------- sei-db/db_engine/view/read_cache_test.go | 5 +- sei-db/db_engine/view/shard.go | 18 +++--- sei-db/db_engine/view/shard_test.go | 18 +++--- sei-db/db_engine/view/test_helpers_test.go | 9 +++ sei-db/db_engine/view/view_manager_config.go | 3 +- sei-db/db_engine/view/view_manager_impl.go | 12 +++- 7 files changed, 78 insertions(+), 46 deletions(-) diff --git a/sei-db/db_engine/view/read_cache.go b/sei-db/db_engine/view/read_cache.go index 831ae87b86..24c6862c3f 100644 --- a/sei-db/db_engine/view/read_cache.go +++ b/sei-db/db_engine/view/read_cache.go @@ -399,6 +399,10 @@ func (e *cacheEntry) injectValueUnlocked(key []byte, ch chan readResult, result c := e.cache c.lock.Lock() + // The failure to report to the manager. The read error wins when both happen, since it is the one + // the waiter is about to be handed. + failure := result.err + if e.status == statusScheduled { if result.err != nil { // Terminal state so readers already waiting on this entry are not stranded. The manager @@ -407,10 +411,10 @@ func (e *cacheEntry) injectValueUnlocked(key []byte, ch chan readResult, result e.setTerminalEntryStateWLocked(key, statusFailed, nil) } else if result.value == nil { e.setTerminalEntryStateWLocked(key, statusDeleted, nil) - c.evictWLocked(c.hardCap()) + failure = c.evictWLocked(c.hardCap()) } else { e.setTerminalEntryStateWLocked(key, statusAvailable, result.value) - c.evictWLocked(c.hardCap()) + failure = c.evictWLocked(c.hardCap()) } } @@ -426,8 +430,8 @@ func (e *cacheEntry) injectValueUnlocked(key []byte, ch chan readResult, result // nobody may be blocked on us while we wait for it. ch <- result - if result.err != nil { - c.reportReadFailure(result.err) + if failure != nil { + c.reportReadFailure(failure) } } @@ -462,7 +466,9 @@ func (c *readCache) bulkInjectValuesUnlocked(reads []pendingRead) { if failure != nil { c.TakeOutOfServiceWLocked(failure) } - c.evictWLocked(c.hardCap()) + if err := c.evictWLocked(c.hardCap()); err != nil && failure == nil { + failure = err + } c.lock.Unlock() // The waiters for this batch were already released by ResolveBatch, so there is nobody blocked @@ -494,7 +500,7 @@ func (c *readCache) entryOrCreateWLocked(key []byte) *cacheEntry { // PutRetiredWLocked installs data retired out of the shard's MVCC layer. A nil value marks the // key as known-deleted (the manager-wide tombstone convention); any other value is cached as // available. Inserts everything, then evicts overflow once at the end. -func (c *readCache) PutRetiredWLocked(data map[string][]byte) { +func (c *readCache) PutRetiredWLocked(data map[string][]byte) error { for k, v := range data { if v == nil { c.deleteRetiredWLocked([]byte(k)) @@ -506,7 +512,7 @@ func (c *readCache) PutRetiredWLocked(data map[string][]byte) { // These insertions may have caused the cache to exceed its size budget, do necessary // evictions. setRetiredWLocked does not evict on its own, so this is the enforcement point // for the bulk insert above. - c.evictWLocked(c.hardCap()) + return c.evictWLocked(c.hardCap()) } // Set a retired value. @@ -559,9 +565,8 @@ func (c *readCache) untrackWLocked(key string, entry *cacheEntry) { } // evictWLocked evicts entries until the cache is within the given budget, choosing each victim as the -// oldest of a small sample. Only entries counted toward the budget are eligible; entries that are not -// still count against the sample. Falls short of the budget when the sample holds no eligible entry. -func (c *readCache) evictWLocked(budget uint64) { +// oldest of a small sample of candidates. +func (c *readCache) evictWLocked(budget uint64) error { for c.trackedBytes > budget { var victimKey string var victim *cacheEntry @@ -569,28 +574,34 @@ func (c *readCache) evictWLocked(budget uint64) { var visited uint64 for key, entry := range c.entries { - visited++ - // An entry holding no value has nothing to reclaim and no stamp worth comparing, but it has - // still consumed a step of the walk. - if entry.size > 0 { - if stamp := entry.lastRead.Load(); stamp <= oldest { - oldest, victimKey, victim = stamp, key, entry - } + if entry.status == statusScheduled { + // A read still in flight must not be evicted: untracking leaves its status untouched, + // so the completing read would re-track an entry no longer in the map. + continue + } + + stamp := entry.lastRead.Load() + if stamp <= oldest { + oldest, victimKey, victim = stamp, key, entry } + + visited++ if visited == c.config.EvictionSampleSize { break } } if victim == nil { - // The walk found nothing to evict, either because the sample happened to hold no values or - // because the accounting disagrees with the map. Stopping is the safe response either way: - // running over budget costs memory, whereas looping here would spin while holding the shard - // lock. The next maintenance pass samples a different part of the map and makes progress. - return + // Unreachable while the accounting agrees with the map. + err := fmt.Errorf("read cache accounting is corrupt: %d tracked bytes exceed budget %d, "+ + "but none of the %d entries is an eviction candidate", + c.trackedBytes, budget, len(c.entries)) + c.TakeOutOfServiceWLocked(err) + return err } c.untrackWLocked(victimKey, victim) } + return nil } // SizeInfoRLocked returns the current size (bytes) and entry count. @@ -604,9 +615,9 @@ func (c *readCache) hardCap() uint64 { } // MaintainWLocked advances the epoch and brings the cache back within its size budget. -func (c *readCache) MaintainWLocked() { +func (c *readCache) MaintainWLocked() error { c.epoch++ - c.evictWLocked(c.maxSize) + return c.evictWLocked(c.maxSize) } // ErrIfOutOfServiceRLocked returns a second-hand error if this cache has been taken out of service, or nil diff --git a/sei-db/db_engine/view/read_cache_test.go b/sei-db/db_engine/view/read_cache_test.go index 5208d966c4..c1ddaf5b7b 100644 --- a/sei-db/db_engine/view/read_cache_test.go +++ b/sei-db/db_engine/view/read_cache_test.go @@ -89,7 +89,8 @@ func TestValueChannelAttachedOnlyWhileScheduled(t *testing.T) { entry := outcome.entry raced := entry.status != statusScheduled shard.lock.Unlock() - entry.injectValueUnlocked([]byte(key), outcome.valueChan, readResult{value: randomValue()}) + entry.injectValueUnlocked( + []byte(key), outcome.valueChan, readResult{value: randomValue()}) shard.lock.Lock() if raced { racedCompletions++ @@ -139,7 +140,7 @@ func TestMaintenanceEvictsBackToBudget(t *testing.T) { require.Greater(t, overBudget, uint64(maxSize)) require.LessOrEqual(t, overBudget, hardCap) - shard.Commit() + commitShard(t, shard) bytes, entries := shard.GetSizeInfo() require.LessOrEqual(t, bytes, uint64(maxSize)) diff --git a/sei-db/db_engine/view/shard.go b/sei-db/db_engine/view/shard.go index 5f98c27d67..c78bc55feb 100644 --- a/sei-db/db_engine/view/shard.go +++ b/sei-db/db_engine/view/shard.go @@ -443,9 +443,10 @@ func (s *shard) Delete(key []byte) error { } // Commit seals the current version; all future updates will be applied to the next version. It also -// runs the read cache's once-per-block maintenance. -// The value returned is the new version number (for sanity checking). -func (s *shard) Commit() uint64 { +// runs the read cache's once-per-block maintenance, whose failure it returns. +// The version returned is the new version number (for sanity checking), and is returned even alongside +// an error so the caller can report both. +func (s *shard) Commit() (uint64, error) { s.lock.Lock() newVersion := s.currentVersion + 1 @@ -455,11 +456,11 @@ func (s *shard) Commit() uint64 { // Sealing a version is the once-per-block moment the read cache does its eviction, so that no read // has to pay for it. - s.cache.MaintainWLocked() + err := s.cache.MaintainWLocked() s.lock.Unlock() - return newVersion + return newVersion, err } // Get the diffs for a range of versions [firstVersion, lastVersion). The returned data should not be mutated @@ -597,12 +598,13 @@ func (s *shard) DropVersions( // Push the combined data down into the read cache, still under the same lock grab, so // readers never observe an intermediate state between the deque cleanup and the cache // insert. - s.cache.PutRetiredWLocked(combinedData) + retireErr := s.cache.PutRetiredWLocked(combinedData) - // Update the oldest version. + // Advanced even when the insert reported a failure: the keys have already moved into the cache, so + // leaving oldestVersion behind would describe a migration that did not happen. s.oldestVersion = lastVersion - return nil + return retireErr } // TakeOutOfService stops this shard from serving reads and accepting writes, reporting err as the diff --git a/sei-db/db_engine/view/shard_test.go b/sei-db/db_engine/view/shard_test.go index e8a2be39e2..8c7385607d 100644 --- a/sei-db/db_engine/view/shard_test.go +++ b/sei-db/db_engine/view/shard_test.go @@ -9,7 +9,7 @@ import ( func TestShardVersionedReads(t *testing.T) { s := newTestShard(t, 4096, newTestDB(nil)) require.NoError(t, s.Set([]byte("k"), []byte("v1"))) - require.Equal(t, uint64(2), s.Commit()) // seals v1, live -> v2 + require.Equal(t, uint64(2), commitShard(t, s)) // seals v1, live -> v2 require.NoError(t, s.Set([]byte("k"), []byte("v2"))) for _, tc := range []struct { @@ -26,8 +26,8 @@ func TestShardVersionedReads(t *testing.T) { func TestShardGetMostRecentValueAtOrBelowVersion(t *testing.T) { s := newTestShard(t, 4096, newTestDB(nil)) require.NoError(t, s.Set([]byte("k"), []byte("v1"))) - _ = s.Commit() // v2 - _ = s.Commit() // v3; no write at v2 + commitShard(t, s) // v2 + commitShard(t, s) // v3; no write at v2 require.NoError(t, s.Set([]byte("k"), []byte("v3"))) // Reading at v2 (no write there) returns v1 (highest version <= 2). @@ -52,9 +52,9 @@ func TestShardValidateVersionOverflow(t *testing.T) { func TestShardGetDiffsForVersions(t *testing.T) { s := newTestShard(t, 4096, newTestDB(nil)) require.NoError(t, s.Set([]byte("a"), []byte("1"))) - _ = s.Commit() // seals v1, live -> v2 + commitShard(t, s) // seals v1, live -> v2 require.NoError(t, s.Set([]byte("b"), []byte("2"))) - _ = s.Commit() // seals v2, live -> v3 (GetDiffs only covers sealed versions) + commitShard(t, s) // seals v2, live -> v3 (GetDiffs only covers sealed versions) diffs, err := s.GetDiffsForVersions(1, 3) // [1, 3) => versions 1 and 2 require.NoError(t, err) @@ -65,7 +65,7 @@ func TestShardGetDiffsForVersions(t *testing.T) { func TestShardGetDiffsForVersionsRejectsBadRange(t *testing.T) { s := newTestShard(t, 4096, newTestDB(nil)) - _ = s.Commit() // oldest=1, current=2 + commitShard(t, s) // oldest=1, current=2 _, err := s.GetDiffsForVersions(3, 1) require.Error(t, err, "firstVersion > lastVersion") @@ -89,9 +89,9 @@ func TestShardDeleteWritesTombstone(t *testing.T) { func TestShardDropVersionsPushesLatestToDB(t *testing.T) { s := newTestShard(t, 4096, newTestDB(nil)) require.NoError(t, s.Set([]byte("k"), []byte("v1"))) - _ = s.Commit() // v2 + commitShard(t, s) // v2 require.NoError(t, s.Set([]byte("k"), []byte("v2"))) - _ = s.Commit() // v3 + commitShard(t, s) // v3 // Drop versions [1, 3): their data collapses into the dbCache, latest value winning. require.NoError(t, s.DropVersions(1, 3)) @@ -104,7 +104,7 @@ func TestShardDropVersionsPushesLatestToDB(t *testing.T) { func TestShardDropVersionsRejectsBadRange(t *testing.T) { s := newTestShard(t, 4096, newTestDB(nil)) - _ = s.Commit() + commitShard(t, s) require.Error(t, s.DropVersions(2, 1)) // first >= last require.Error(t, s.DropVersions(2, 3)) // first != oldest } diff --git a/sei-db/db_engine/view/test_helpers_test.go b/sei-db/db_engine/view/test_helpers_test.go index da5e7553c5..541a764fea 100644 --- a/sei-db/db_engine/view/test_helpers_test.go +++ b/sei-db/db_engine/view/test_helpers_test.go @@ -379,6 +379,15 @@ func awaitRetired(t *testing.T, manager ViewManager, version uint64) { }, 2*time.Second, 2*time.Millisecond, "version %d was not retired in time", version) } +// commitShard seals the shard's current version, failing the test if its once-per-block cache +// maintenance reported a failure. Returns the new version number. +func commitShard(t *testing.T, s *shard) uint64 { + t.Helper() + version, err := s.Commit() + require.NoError(t, err) + return version +} + // openIteratorCount reports how many iterators are currently open on the manager. Every iterator // registers with every shard, so any one shard's count is the manager's count. func openIteratorCount(manager ViewManager) uint64 { diff --git a/sei-db/db_engine/view/view_manager_config.go b/sei-db/db_engine/view/view_manager_config.go index 7cef4dc39a..b227c9eb91 100644 --- a/sei-db/db_engine/view/view_manager_config.go +++ b/sei-db/db_engine/view/view_manager_config.go @@ -132,7 +132,8 @@ func (c *ViewManagerConfig) Validate() error { if c.EvictionSlackDivisor == 0 { return fmt.Errorf("EvictionSlackDivisor must be greater than 0") } - // Zero would leave eviction unable to find a victim, so the cache would grow without bound. + // Zero never matches evictWLocked's sample counter, so every eviction would walk the whole entry + // map under the write lock rather than a bounded sample. if c.EvictionSampleSize == 0 { return fmt.Errorf("EvictionSampleSize must be greater than 0") } diff --git a/sei-db/db_engine/view/view_manager_impl.go b/sei-db/db_engine/view/view_manager_impl.go index 0e4fedd0a7..b2124413d7 100644 --- a/sei-db/db_engine/view/view_manager_impl.go +++ b/sei-db/db_engine/view/view_manager_impl.go @@ -401,8 +401,16 @@ func (c *viewManager) Commit() (View, error) { c.metrics.setViewPhase("shards_view") - for _, shard := range c.shards { - shardVersion := shard.Commit() + for i, shard := range c.shards { + shardVersion, err := shard.Commit() + if err != nil { + // The shard sealed its version but its read cache could not be maintained, which means the + // cache can no longer account for its own contents. Bricked for the same reason as below: + // the failure must be latched rather than leaving the manager callable. + err = fmt.Errorf("failed to maintain the read cache of shard %d: %w", i, err) + c.brickLocked(err) + return nil, err + } if shardVersion != c.currentVersion { // Should be impossible. The manager is now inconsistent (some shards committed, some // not), so brick it: the failure must be latched and every subsequent call must fail,