cache: don't mark blocks as accessed for background work

We don't want block cache hits for compactions to cause the respective
block to stay in the cache longer (in fact, it is about to become
obsolete). This change adds a `Peek()` method to the block cache which
is like `Get()` but does not mark the block as recently accessed.

Author: RaduBerinde

db_test.go

@@ -858,7 +858,7 @@ func TestMemTableReservation(t *testing.T) {
 		t.Fatalf("expected 2 refs, but found %d", refs)
 	}
 	// Verify the memtable reservation has caused our test block to be evicted.
-	if cv := tmpHandle.Get(base.DiskFileNum(0), 0); cv != nil {
+	if cv := tmpHandle.Peek(base.DiskFileNum(0), 0); cv != nil {
 		t.Fatalf("expected failure, but found success: %#v", cv)
 	}
 

internal/cache/cache.go

@@ -259,15 +259,19 @@ func (c *Handle) Cache() *Cache {
 	return c.cache
 }
 
+// Peek retrieves the cache value for the specified file and offset, returning
+// nil if no value is present. Peek does not affect the state of the cache (it
+// does not "count" as an access as far as the cache replacement is concerned).
+func (c *Handle) Peek(fileNum base.DiskFileNum, offset uint64) *Value {
+	k := makeKey(c.id, fileNum, offset)
+	return c.cache.getShard(k).get(k, true /* peekOnly */)
+}
+
 // Get retrieves the cache value for the specified file and offset, returning
 // nil if no value is present.
 func (c *Handle) Get(fileNum base.DiskFileNum, offset uint64) *Value {
 	k := makeKey(c.id, fileNum, offset)
-	cv, re := c.cache.getShard(k).getWithMaybeReadEntry(k, false /* desireReadEntry */)
-	if invariants.Enabled && re != nil {
-		panic("readEntry should be nil")
-	}
-	return cv
+	return c.cache.getShard(k).get(k, false /* peekOnly */)
 }
 
 // GetWithReadHandle retrieves the cache value for the specified handleID, fileNum
@@ -304,7 +308,7 @@ func (c *Handle) GetWithReadHandle(
 	err error,
 ) {
 	k := makeKey(c.id, fileNum, offset)
-	cv, re := c.cache.getShard(k).getWithMaybeReadEntry(k, true /* desireReadEntry */)
+	cv, re := c.cache.getShard(k).getWithReadEntry(k)
 	if cv != nil {
 		return cv, ReadHandle{}, 0, 0, true, nil
 	}

internal/cache/cache_test.go

@@ -69,6 +69,46 @@ func setTestValue(c *Handle, fileNum base.DiskFileNum, offset uint64, s string,
 	v.Release()
 }
 
+// TestCachePeek verifies that Peek does not update the recently accessed status
+// of an entry.
+func TestCachePeek(t *testing.T) {
+	const size = 10
+	cache := NewWithShards(size, 1)
+	defer cache.Unref()
+	h := cache.NewHandle()
+	defer h.Close()
+
+	for i := range size {
+		setTestValue(h, 0, uint64(i), "a", 1)
+	}
+	for i := range size / 2 {
+		v := h.Get(base.DiskFileNum(0), uint64(i))
+		if v == nil {
+			t.Fatalf("expected to find block %d", i)
+		}
+		v.Release()
+	}
+	for i := size / 2; i < size; i++ {
+		v := h.Peek(base.DiskFileNum(0), uint64(i))
+		if v == nil {
+			t.Fatalf("expected to find block %d", i)
+		}
+		v.Release()
+	}
+	// Now add more entries to cause eviction.
+	for i := range size / 4 {
+		setTestValue(h, 0, uint64(size+i), "a", 1)
+	}
+	// Verify that the Gets still find their values, despite the Peeks.
+	for i := range size / 2 {
+		v := h.Get(base.DiskFileNum(0), uint64(i))
+		if v == nil {
+			t.Fatalf("expected to find block %d", i)
+		}
+		v.Release()
+	}
+}
+
 func TestCacheDelete(t *testing.T) {
 	cache := NewWithShards(100, 1)
 	defer cache.Unref()

internal/cache/clockpro.go

@@ -130,32 +130,50 @@ func (c *shard) init(maxSize int64) {
 	c.readShard.Init(c)
 }
 
-// getWithMaybeReadEntry is the internal helper for implementing
+// get returns the cache value if it exists.
+//
+// If peekOnly is true, the state of the cache is not modified to reflect the
+// access.
+func (c *shard) get(k key, peekOnly bool) *Value {
+	c.mu.RLock()
+	if e, _ := c.blocks.Get(k); e != nil {
+		if value := e.acquireValue(); value != nil {
+			// Note: we Load first to avoid an atomic XCHG when not necessary.
+			if !peekOnly && !e.referenced.Load() {
+				e.referenced.Store(true)
+			}
+			c.mu.RUnlock()
+			c.hits.Add(1)
+			return value
+		}
+	}
+	c.mu.RUnlock()
+	c.misses.Add(1)
+	return nil
+}
+
+// getWithReadEntry is the internal helper for implementing
 // Cache.{Get,GetWithReadHandle}. When desireReadEntry is true, and the block
 // is not in the cache (nil Value), a non-nil readEntry is returned (in which
 // case the caller is responsible to dereference the entry, via one of
 // unrefAndTryRemoveFromMap(), setReadValue(), setReadError()).
-func (c *shard) getWithMaybeReadEntry(k key, desireReadEntry bool) (*Value, *readEntry) {
+func (c *shard) getWithReadEntry(k key) (*Value, *readEntry) {
 	c.mu.RLock()
-	var value *Value
 	if e, _ := c.blocks.Get(k); e != nil {
-		value = e.acquireValue()
-		// Note: we Load first to avoid an atomic XCHG when not necessary.
-		if value != nil && !e.referenced.Load() {
-			e.referenced.Store(true)
+		if value := e.acquireValue(); value != nil {
+			// Note: we Load first to avoid an atomic XCHG when not necessary.
+			if !e.referenced.Load() {
+				e.referenced.Store(true)
+			}
+			c.mu.RUnlock()
+			c.hits.Add(1)
+			return value, nil
 		}
 	}
-	var re *readEntry
-	if value == nil && desireReadEntry {
-		re = c.readShard.acquireReadEntry(k)
-	}
+	re := c.readShard.acquireReadEntry(k)
 	c.mu.RUnlock()
-	if value == nil {
-		c.misses.Add(1)
-	} else {
-		c.hits.Add(1)
-	}
-	return value, re
+	c.misses.Add(1)
+	return nil, re
 }
 
 func (c *shard) set(k key, value *Value, markAccessed bool) {

internal/cache/read_shard_test.go

@@ -50,7 +50,7 @@ func newTestReader(
 }
 
 func (r *testReader) getAsync(shard *shard) *string {
-	v, re := shard.getWithMaybeReadEntry(r.key, true /* desireReadEntry */)
+	v, re := shard.getWithReadEntry(r.key)
 	if v != nil {
 		str := string(v.RawBuffer())
 		v.Release()

sstable/block/block.go

@@ -366,7 +366,7 @@ func (r *Reader) Read(
 	// reading a block.
 	if r.opts.CacheOpts.CacheHandle == nil || env.BufferPool != nil {
 		if r.opts.CacheOpts.CacheHandle != nil {
-			if cv := r.opts.CacheOpts.CacheHandle.Get(r.opts.CacheOpts.FileNum, bh.Offset); cv != nil {
+			if cv := r.opts.CacheOpts.CacheHandle.Peek(r.opts.CacheOpts.FileNum, bh.Offset); cv != nil {
 				recordCacheHit(ctx, env, readHandle, bh, kind)
 				return CacheBufferHandle(cv), nil
 			}
@@ -528,12 +528,13 @@ func (r *Reader) Readable() objstorage.Readable {
 	return r.readable
 }
 
-// GetFromCache retrieves the block from the cache, if it is present.
+// GetFromCache retrieves the block from the cache, if it is present. It does
+// not mark the block as recently used.
 //
 // Users should prefer using Read, which handles reading from object storage on
 // a cache miss.
 func (r *Reader) GetFromCache(bh Handle) *cache.Value {
-	return r.opts.CacheOpts.CacheHandle.Get(r.opts.CacheOpts.FileNum, bh.Offset)
+	return r.opts.CacheOpts.CacheHandle.Peek(r.opts.CacheOpts.FileNum, bh.Offset)
 }
 
 // UsePreallocatedReadHandle returns a ReadHandle that reads from the reader and