From bc371d4f4abc71d996b4ae698266bcec25094ab4 Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Wed, 26 Jul 2023 10:40:48 -0400 Subject: [PATCH] internal/cache: reduce mutex contention from EvictFile We've observed block cache mutex contention originating from EvictFile. When evicting a file with a significant count of blocks currently held within the block cache, EvictFile may hold the block cache shard mutex for an extended duration, increasing latency for requests that must access the same shard. This commit mitigates this contention with two approaches. EvictFile now periodically drops the mutex to provide these other requests with an opportunity to make progress. Additionally, rather than free()-ing manually managed memory back to the memory allocator while holding the mutex, the free is deferred until the shard mutex has been dropped. Informs #1997. --- internal/cache/clockpro.go | 96 +++++++++++++++++++++++++++----------- internal/cache/entry.go | 4 +- internal/cache/value.go | 2 +- 3 files changed, 70 insertions(+), 32 deletions(-) diff --git a/internal/cache/clockpro.go b/internal/cache/clockpro.go index 94102a274b..7a97700c57 100644 --- a/internal/cache/clockpro.go +++ b/internal/cache/clockpro.go @@ -71,9 +71,7 @@ func (h Handle) Get() []byte { // Release releases the reference to the cache entry. func (h Handle) Release() { - if h.value != nil { - h.value.release() - } + h.value.release() } type shard struct { @@ -176,7 +174,7 @@ func (c *shard) Set(id uint64, fileNum base.DiskFileNum, offset uint64, value *V // cache entry was a test page c.sizeTest -= e.size c.countTest-- - c.metaDel(e) + c.metaDel(e).release() c.metaCheck(e) e.size = int64(len(value.buf)) @@ -233,37 +231,75 @@ func (c *shard) Delete(id uint64, fileNum base.DiskFileNum, offset uint64) { return } - c.mu.Lock() - defer c.mu.Unlock() + var deletedValue *Value + func() { + c.mu.Lock() + defer c.mu.Unlock() - e := c.blocks.Get(k) - if e == nil { - return - } - c.metaEvict(e) - - c.checkConsistency() + e := c.blocks.Get(k) + if e == nil { + return + } + deletedValue = c.metaEvict(e) + c.checkConsistency() + }() + // Now that the mutex has been dropped, release the reference which will + // potentially free the memory associated with the previous cached value. + deletedValue.release() } // EvictFile evicts all of the cache values for the specified file. func (c *shard) EvictFile(id uint64, fileNum base.DiskFileNum) { + fkey := key{fileKey{id, fileNum}, 0} + for c.evictFileRun(fkey) { + // Sched switch to give another goroutine an opportunity to acquire the + // shard mutex. + runtime.Gosched() + } +} + +func (c *shard) evictFileRun(fkey key) (moreRemaining bool) { + // If most of the file's blocks are held in the block cache, evicting all + // the blocks may take a while. We don't want to block the entire cache + // shard, forcing concurrent readers to wait until we're finished. We drop + // the mutex every [blocksPerMutexAcquisition] blocks to give other + // goroutines an opportunity to make progress. + const blocksPerMutexAcquisition = 5 c.mu.Lock() - defer c.mu.Unlock() - fkey := key{fileKey{id, fileNum}, 0} + // Releasing a value may result in free-ing it back to the memory allocator. + // This can have a nontrivial cost that we'd prefer to not pay while holding + // the shard mutex, so we collect the evicted values in a local slice and + // only release them in a defer after dropping the cache mutex. + var obsoleteValuesAlloc [blocksPerMutexAcquisition]*Value + obsoleteValues := obsoleteValuesAlloc[:0] + defer func() { + c.mu.Unlock() + for _, v := range obsoleteValues { + v.release() + } + }() + blocks := c.files.Get(fkey) if blocks == nil { - return + // No blocks for this file. + return false } - for b, n := blocks, (*entry)(nil); ; b = n { + + // b is the current head of the doubly linked list, and n is the entry after b. + for b, n := blocks, (*entry)(nil); len(obsoleteValues) < cap(obsoleteValues); b = n { n = b.fileLink.next - c.metaEvict(b) + obsoleteValues = append(obsoleteValues, c.metaEvict(b)) if b == n { - break + // b == n represents the case where b was the last entry remaining + // in the doubly linked list, which is why it pointed at itself. So + // no more entries left. + c.checkConsistency() + return false } } - - c.checkConsistency() + // Exhausted blocksPerMutexAcquisition. + return true } func (c *shard) Free() { @@ -274,7 +310,7 @@ func (c *shard) Free() { // metaCheck call when the "invariants" build tag is specified. for c.handHot != nil { e := c.handHot - c.metaDel(c.handHot) + c.metaDel(c.handHot).release() e.free() } @@ -359,12 +395,14 @@ func (c *shard) metaAdd(key key, e *entry) bool { // Remove the entry from the cache. This removes the entry from the blocks map, // the files map, and ensures that hand{Hot,Cold,Test} are not pointing at the -// entry. -func (c *shard) metaDel(e *entry) { +// entry. Returns the deleted value that must be released, if any. +func (c *shard) metaDel(e *entry) (deletedValue *Value) { if value := e.peekValue(); value != nil { value.ref.trace("metaDel") } - e.setValue(nil) + // Remove the pointer to the value. + deletedValue = e.val + e.val = nil c.blocks.Delete(e.key) if entriesGoAllocated { @@ -396,6 +434,7 @@ func (c *shard) metaDel(e *entry) { } else { c.files.Put(fkey, next) } + return deletedValue } // Check that the specified entry is not referenced by the cache. @@ -455,7 +494,7 @@ func (c *shard) metaCheck(e *entry) { } } -func (c *shard) metaEvict(e *entry) { +func (c *shard) metaEvict(e *entry) (evictedValue *Value) { switch e.ptype { case etHot: c.sizeHot -= e.size @@ -467,9 +506,10 @@ func (c *shard) metaEvict(e *entry) { c.sizeTest -= e.size c.countTest-- } - c.metaDel(e) + evictedValue = c.metaDel(e) c.metaCheck(e) e.free() + return evictedValue } func (c *shard) evict() { @@ -564,7 +604,7 @@ func (c *shard) runHandTest() { if c.coldTarget < 0 { c.coldTarget = 0 } - c.metaDel(e) + c.metaDel(e).release() c.metaCheck(e) e.free() } diff --git a/internal/cache/entry.go b/internal/cache/entry.go index 28a3a3f293..a49fde6b7e 100644 --- a/internal/cache/entry.go +++ b/internal/cache/entry.go @@ -139,9 +139,7 @@ func (e *entry) setValue(v *Value) { } old := e.val e.val = v - if old != nil { - old.release() - } + old.release() } func (e *entry) peekValue() *Value { diff --git a/internal/cache/value.go b/internal/cache/value.go index c5c967aff3..6d2cae15e5 100644 --- a/internal/cache/value.go +++ b/internal/cache/value.go @@ -40,7 +40,7 @@ func (v *Value) acquire() { } func (v *Value) release() { - if v.ref.release() { + if v != nil && v.ref.release() { v.free() } }