Skip to content

Commit

Permalink
db: fix regression in table cache hot path
Browse files Browse the repository at this point in the history
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 cockroachdb/cockroach#115215
  • Loading branch information
RaduBerinde committed Dec 6, 2023
1 parent a59eb68 commit b3f359e
Showing 1 changed file with 17 additions and 14 deletions.
31 changes: 17 additions & 14 deletions table_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -755,35 +755,38 @@ 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}
if n := c.mu.nodes[key]; n != nil && n.value != nil {
// 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)
Expand All @@ -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)
Expand All @@ -834,7 +837,7 @@ func (c *tableCacheShard) findNode(

c.misses.Add(1)

v = &tableCacheValue{
v := &tableCacheValue{
loaded: make(chan struct{}),
}
v.refCount.Store(2)
Expand Down

0 comments on commit b3f359e

Please sign in to comment.