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 cockroachdb#1997.

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,71 @@ 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 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() {
+		if !moreRemaining {
+			c.checkConsistency()
+		}
+		c.mu.Unlock()
+		for _, v := range obsoleteValues {
+			v.release()
+		}
+	}()
+
 	blocks := c.files.Get(fkey)
 	if blocks == nil {
-		return
+		return false
 	}
-	for b, n := blocks, (*entry)(nil); ; b = n {
+	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
+			return false
 		}
 	}
-
-	c.checkConsistency()
+	// Exhausted blocksPerMutexAcquisition.
+	return true
 }
 
 func (c *shard) Free() {
@@ -274,7 +306,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 +391,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 +430,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 +490,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 +502,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 +600,7 @@ func (c *shard) runHandTest() {
 		if c.coldTarget < 0 {
 			c.coldTarget = 0
 		}
-		c.metaDel(e)
+		c.metaDel(e).release()
 		c.metaCheck(e)
 		e.free()
 	}

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 {

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