From b3f359e3a5034f2e066276567dae5da9de81e8d9 Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Wed, 6 Dec 2023 09:31:39 -0800
Subject: [PATCH] db: fix regression in table cache hot path

A seemingly innocuous change in `tableCacheShard.findNode` resulted in
a new allocation. We named the return variable, then we captured it in
the `closeHook` callback, which I guess means that the pointer itself
needs to be put on the heap.

With this change `BenchmarkTableCacheHotPath` goes down from 1
alloc/op to 0, and the CRDB
`RefreshRange/linear-keys/refresh_window=[99.00,99.00]` benchmark goes
from 101 allocs/op to 61.

Informs https://github.com/cockroachdb/cockroach/issues/115215
---
 table_cache.go | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/table_cache.go b/table_cache.go
index 7a1a63643b..c4e8bf190e 100644
--- a/table_cache.go
+++ b/table_cache.go
@@ -755,27 +755,30 @@ func (c *tableCacheShard) unrefValue(v *tableCacheValue) {
 // findNode returns the node for the table with the given file number, creating
 // that node if it didn't already exist. The caller is responsible for
 // decrementing the returned node's refCount.
-func (c *tableCacheShard) findNode(
-	meta *fileMetadata, dbOpts *tableCacheOpts,
-) (v *tableCacheValue) {
+func (c *tableCacheShard) findNode(meta *fileMetadata, dbOpts *tableCacheOpts) *tableCacheValue {
+	v := c.findNodeInternal(meta, dbOpts)
+
 	// Loading a file before its global sequence number is known (eg,
 	// during ingest before entering the commit pipeline) can pollute
 	// the cache with incorrect state. In invariant builds, verify
 	// that the global sequence number of the returned reader matches.
 	if invariants.Enabled {
-		defer func() {
-			if v.reader != nil && meta.LargestSeqNum == meta.SmallestSeqNum &&
-				v.reader.Properties.GlobalSeqNum != meta.SmallestSeqNum {
-				panic(errors.AssertionFailedf("file %s loaded from table cache with the wrong global sequence number %d",
-					meta, v.reader.Properties.GlobalSeqNum))
-			}
-		}()
+		if v.reader != nil && meta.LargestSeqNum == meta.SmallestSeqNum &&
+			v.reader.Properties.GlobalSeqNum != meta.SmallestSeqNum {
+			panic(errors.AssertionFailedf("file %s loaded from table cache with the wrong global sequence number %d",
+				meta, v.reader.Properties.GlobalSeqNum))
+		}
 	}
+	return v
+}
+
+func (c *tableCacheShard) findNodeInternal(
+	meta *fileMetadata, dbOpts *tableCacheOpts,
+) *tableCacheValue {
 	if refs := meta.Refs(); refs <= 0 {
 		panic(errors.AssertionFailedf("attempting to load file %s with refs=%d from table cache",
 			meta, refs))
 	}
-
 	// Fast-path for a hit in the cache.
 	c.mu.RLock()
 	key := tableCacheKey{dbOpts.cacheID, meta.FileBacking.DiskFileNum}
@@ -783,7 +786,7 @@ func (c *tableCacheShard) findNode(
 		// Fast-path hit.
 		//
 		// The caller is responsible for decrementing the refCount.
-		v = n.value
+		v := n.value
 		v.refCount.Add(1)
 		c.mu.RUnlock()
 		n.referenced.Store(true)
@@ -810,7 +813,7 @@ func (c *tableCacheShard) findNode(
 		// Slow-path hit of a hot or cold node.
 		//
 		// The caller is responsible for decrementing the refCount.
-		v = n.value
+		v := n.value
 		v.refCount.Add(1)
 		n.referenced.Store(true)
 		c.hits.Add(1)
@@ -834,7 +837,7 @@ func (c *tableCacheShard) findNode(
 
 	c.misses.Add(1)
 
-	v = &tableCacheValue{
+	v := &tableCacheValue{
 		loaded: make(chan struct{}),
 	}
 	v.refCount.Store(2)