Repository: cockroachdb/pebble
Pull request #4117 (2024-10-27 21:08:23)

invariants, cache: reorganize and simplify tag-dependent code
#### invariants: reorganize code

We add a `buildtags` package which defines constants for the `race`,
`cgo`, `tracing` tags and we simplify the `invariants` package.

#### cache: simplify value code

We have three tag-dependent file variants for allocating and freeing
`Value`.  This commit greatly simplifies this code using constants
instead of build tags and consolidating the non-cgo and the invariant
implementations.

#### cache: simplify entry code

Consolidate the entry allocation paths using constants instead of
build tags.

cockroach-teamcity: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/cockroachdb/pebble/4117)
<!-- Reviewable:end -->


Patch:
diff --git a/checkpoint.go b/checkpoint.go
index fb421149..5f599bd6 100644
--- a/checkpoint.go
+++ b/checkpoint.go
@@ -15,7 +15,6 @@ import (
 	"github.com/cockroachdb/pebble/record"
 	"github.com/cockroachdb/pebble/vfs"
 	"github.com/cockroachdb/pebble/vfs/atomicfs"
-	"github.com/cockroachdb/pebble/wal"
 )
 
 // checkpointOptions hold the optional parameters to construct checkpoint
@@ -186,9 +185,7 @@ func (d *DB) Checkpoint(
 	// we read, otherwise we might copy a versionEdit not reflected in the
 	// sstables we copy/link.
 	d.mu.versions.logLock()
-	// Get the unflushed log files, the current version, and the current manifest
-	// file number.
-	memQueue := d.mu.mem.queue
+	// Get the the current version and the current manifest file number.
 	current := d.mu.versions.currentVersion()
 	formatVers := d.FormatMajorVersion()
 	manifestFileNum := d.mu.versions.manifestFileNum
@@ -200,22 +197,16 @@ func (d *DB) Checkpoint(
 		virtualBackingFiles[backing.DiskFileNum] = struct{}{}
 	})
 
-	queuedLogNums := make([]wal.NumWAL, 0, len(memQueue))
-	for i := range memQueue {
-		if logNum := memQueue[i].logNum; logNum != 0 {
-			queuedLogNums = append(queuedLogNums, wal.NumWAL(logNum))
-		}
-	}
+	// Acquire the logs while holding mutexes to ensure we don't race with a
+	// flush that might mark a log that's relevant to `current` as obsolete
+	// before our call to List.
+	allLogicalLogs := d.mu.log.manager.List()
+
 	// Release the manifest and DB.mu so we don't block other operations on
 	// the database.
 	d.mu.versions.logUnlock()
 	d.mu.Unlock()
 
-	allLogicalLogs, err := d.mu.log.manager.List()
-	if err != nil {
-		return err
-	}
-
 	// Wrap the normal filesystem with one which wraps newly created files with
 	// vfs.NewSyncingFile.
 	fs := vfs.NewSyncingFS(d.opts.FS, vfs.SyncingFileOptions{
@@ -348,12 +339,15 @@ func (d *DB) Checkpoint(
 
 	// Copy the WAL files. We copy rather than link because WAL file recycling
 	// will cause the WAL files to be reused which would invalidate the
-	// checkpoint.
-	for _, logNum := range queuedLogNums {
-		log, ok := allLogicalLogs.Get(logNum)
-		if !ok {
-			return errors.Newf("log %s not found", logNum)
-		}
+	// checkpoint. It's possible allLogicalLogs includes logs that are not
+	// relevant (beneath the version's MinUnflushedLogNum). These extra files
+	// are harmless. The earlier (wal.Manager).List call will not include
+	// obsolete logs that are sitting in the recycler or have already been
+	// passed off to the cleanup manager for deletion.
+	//
+	// TODO(jackson): It would be desirable to copy all recycling and obsolete
+	// WALs to aid corruption postmortem debugging should we need them.
+	for _, log := range allLogicalLogs {
 		for i := 0; i < log.NumSegments(); i++ {
 			srcFS, srcPath := log.SegmentLocation(i)
 			destPath := fs.PathJoin(destDir, srcFS.PathBase(srcPath))
diff --git a/go.mod b/go.mod
index 4e143d2f..652cfbcb 100644
--- a/go.mod
+++ b/go.mod
@@ -4,10 +4,9 @@ require (
 	github.com/DataDog/zstd v1.5.6-0.20230824185856-869dae002e5e
 	github.com/HdrHistogram/hdrhistogram-go v1.1.2
 	github.com/cespare/xxhash/v2 v2.2.0
-	github.com/cockroachdb/crlib v0.0.0-20241030175859-ddcdee82a927
+	github.com/cockroachdb/crlib v0.0.0-20241112164430-1264a2edc35b
 	github.com/cockroachdb/datadriven v1.0.3-0.20240530155848-7682d40af056
 	github.com/cockroachdb/errors v1.11.3
-	github.com/cockroachdb/fifo v0.0.0-20240606204812-0bbfbd93a7ce
 	github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895
 	github.com/cockroachdb/redact v1.1.5
 	github.com/cockroachdb/swiss v0.0.0-20240612210725-f4de07ae6964
diff --git a/go.sum b/go.sum
index ed357f5d..8fcc0ad7 100644
--- a/go.sum
+++ b/go.sum
@@ -71,14 +71,12 @@ github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5P
 github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
-github.com/cockroachdb/crlib v0.0.0-20241030175859-ddcdee82a927 h1:KxHSDcEqwtVRnknERc+ao26hlGQuReg7hfsdQeNoomE=
-github.com/cockroachdb/crlib v0.0.0-20241030175859-ddcdee82a927/go.mod h1:Gq51ZeKaFCXk6QwuGM0w1dnaOqc/F5zKT2zA9D6Xeac=
+github.com/cockroachdb/crlib v0.0.0-20241112164430-1264a2edc35b h1:SHlYZ/bMx7frnmeqCu+xm0TCxXLzX3jQIVuFbnFGtFU=
+github.com/cockroachdb/crlib v0.0.0-20241112164430-1264a2edc35b/go.mod h1:Gq51ZeKaFCXk6QwuGM0w1dnaOqc/F5zKT2zA9D6Xeac=
 github.com/cockroachdb/datadriven v1.0.3-0.20240530155848-7682d40af056 h1:slXychO2uDM6hYRu4c0pD0udNI8uObfeKN6UInWViS8=
 github.com/cockroachdb/datadriven v1.0.3-0.20240530155848-7682d40af056/go.mod h1:a9RdTaap04u637JoCzcUoIcDmvwSUtcUFtT/C3kJlTU=
 github.com/cockroachdb/errors v1.11.3 h1:5bA+k2Y6r+oz/6Z/RFlNeVCesGARKuC6YymtcDrbC/I=
 github.com/cockroachdb/errors v1.11.3/go.mod h1:m4UIW4CDjx+R5cybPsNrRbreomiFqt8o1h1wUVazSd8=
-github.com/cockroachdb/fifo v0.0.0-20240606204812-0bbfbd93a7ce h1:giXvy4KSc/6g/esnpM7Geqxka4WSqI1SZc7sMJFd3y4=
-github.com/cockroachdb/fifo v0.0.0-20240606204812-0bbfbd93a7ce/go.mod h1:9/y3cnZ5GKakj/H4y9r9GTjCvAFta7KLgSHPJJYc52M=
 github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b h1:r6VH0faHjZeQy818SGhaone5OnYfxFR/+AzdY3sf5aE=
 github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b/go.mod h1:Vz9DsVWQQhf3vs21MhPMZpMGSht7O/2vFW2xusFUVOs=
 github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895 h1:XANOgPYtvELQ/h4IrmPAohXqe2pWA8Bwhejr3VQoZsA=
diff --git a/internal/buildtags/cgo_off.go b/internal/buildtags/cgo_off.go
new file mode 100644
index 00000000..4924eec0
--- /dev/null
+++ b/internal/buildtags/cgo_off.go
@@ -0,0 +1,10 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+//go:build !cgo
+
+package buildtags
+
+// Cgo is true if we were built with the "cgo" build tag.
+const Cgo = false
diff --git a/internal/buildtags/cgo_on.go b/internal/buildtags/cgo_on.go
new file mode 100644
index 00000000..fc2da792
--- /dev/null
+++ b/internal/buildtags/cgo_on.go
@@ -0,0 +1,10 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+//go:build cgo
+
+package buildtags
+
+// Cgo is true if we were built with the "cgo" build tag.
+const Cgo = true
diff --git a/internal/buildtags/invariants_off.go b/internal/buildtags/invariants_off.go
new file mode 100644
index 00000000..5880ab5e
--- /dev/null
+++ b/internal/buildtags/invariants_off.go
@@ -0,0 +1,11 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+//go:build invariants
+
+package buildtags
+
+// Invariants indicates if the invariants tag is used.
+// See invariants.Enabled.
+const Invariants = true
diff --git a/internal/buildtags/invariants_on.go b/internal/buildtags/invariants_on.go
new file mode 100644
index 00000000..2e3d26e7
--- /dev/null
+++ b/internal/buildtags/invariants_on.go
@@ -0,0 +1,11 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+//go:build !invariants
+
+package buildtags
+
+// Invariants indicates if the invariants tag is used.
+// See invariants.Enabled.
+const Invariants = false
diff --git a/internal/invariants/race_off.go b/internal/buildtags/race_off.go
similarity index 60%
rename from internal/invariants/race_off.go
rename to internal/buildtags/race_off.go
index b2b8c5e8..73548953 100644
--- a/internal/invariants/race_off.go
+++ b/internal/buildtags/race_off.go
@@ -3,9 +3,8 @@
 // the LICENSE file.
 
 //go:build !race
-// +build !race
 
-package invariants
+package buildtags
 
-// RaceEnabled is true if we were built with the "race" build tag.
-const RaceEnabled = false
+// Race is true if we were built with the "race" build tag.
+const Race = false
diff --git a/internal/invariants/race_on.go b/internal/buildtags/race_on.go
similarity index 60%
rename from internal/invariants/race_on.go
rename to internal/buildtags/race_on.go
index 46613f72..fead86a7 100644
--- a/internal/invariants/race_on.go
+++ b/internal/buildtags/race_on.go
@@ -3,9 +3,8 @@
 // the LICENSE file.
 
 //go:build race
-// +build race
 
-package invariants
+package buildtags
 
-// RaceEnabled is true if we were built with the "race" build tag.
-const RaceEnabled = true
+// Race is true if we were built with the "race" build tag.
+const Race = true
diff --git a/internal/buildtags/tracing_off.go b/internal/buildtags/tracing_off.go
new file mode 100644
index 00000000..d1f48e06
--- /dev/null
+++ b/internal/buildtags/tracing_off.go
@@ -0,0 +1,12 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+//go:build !tracing
+
+package buildtags
+
+// Tracing indicates if the tracing tag is used.
+//
+// This tag enables low-level tracing code in the block cache.
+const Tracing = false
diff --git a/internal/buildtags/tracing_on.go b/internal/buildtags/tracing_on.go
new file mode 100644
index 00000000..defbd5f9
--- /dev/null
+++ b/internal/buildtags/tracing_on.go
@@ -0,0 +1,12 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+//go:build tracing
+
+package buildtags
+
+// Tracing indicates if the tracing tag is used.
+//
+// This tag enables low-level tracing code in the block cache.
+const Tracing = true
diff --git a/internal/cache/cgo_disabled.go b/internal/cache/cgo_disabled.go
deleted file mode 100644
index 0e755745..00000000
--- a/internal/cache/cgo_disabled.go
+++ /dev/null
@@ -1,10 +0,0 @@
-// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build !cgo
-// +build !cgo
-
-package cache
-
-const cgoEnabled = false
diff --git a/internal/cache/cgo_enabled.go b/internal/cache/cgo_enabled.go
deleted file mode 100644
index b7014cb0..00000000
--- a/internal/cache/cgo_enabled.go
+++ /dev/null
@@ -1,10 +0,0 @@
-// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build cgo
-// +build cgo
-
-package cache
-
-const cgoEnabled = true
diff --git a/internal/cache/entry.go b/internal/cache/entry.go
index 5473a867..724cb290 100644
--- a/internal/cache/entry.go
+++ b/internal/cache/entry.go
@@ -4,7 +4,18 @@
 
 package cache
 
-import "sync/atomic"
+import (
+	"fmt"
+	"os"
+	"runtime"
+	"sync"
+	"sync/atomic"
+	"unsafe"
+
+	"github.com/cockroachdb/pebble/internal/buildtags"
+	"github.com/cockroachdb/pebble/internal/invariants"
+	"github.com/cockroachdb/pebble/internal/manual"
+)
 
 type entryType int8
 
@@ -145,3 +156,113 @@ func (e *entry) acquireValue() *Value {
 	}
 	return v
 }
+
+// The entries are normally allocated using the manual package. We use a
+// sync.Pool with each item in the pool holding multiple entries that can be
+// reused.
+//
+// We cannot use manual memory when the Value is allocated using the Go
+// allocator: in this case, we use the Go allocator because we need the entry
+// pointers to the Values to be discoverable by the GC.
+//
+// We also use the Go allocator in race mode because the normal path relies on a
+// finalizer (and historically there have been some finalizer-related bugs in
+// the race detector, in go1.15 and earlier).
+const entriesGoAllocated = valueEntryGoAllocated || buildtags.Race
+
+const entrySize = int(unsafe.Sizeof(entry{}))
+
+func entryAllocNew() *entry {
+	if invariants.UseFinalizers {
+		// We want to allocate each entry independently to check that it has been
+		// properly cleaned up.
+		e := &entry{}
+		invariants.SetFinalizer(e, func(obj interface{}) {
+			e := obj.(*entry)
+			if *e != (entry{}) {
+				fmt.Fprintf(os.Stderr, "%p: entry was not freed", e)
+				os.Exit(1)
+			}
+		})
+		return e
+	}
+	a := entryAllocPool.Get().(*entryAllocCache)
+	e := a.alloc()
+	entryAllocPool.Put(a)
+	return e
+}
+
+func entryAllocFree(e *entry) {
+	if invariants.UseFinalizers {
+		*e = entry{}
+		return
+	}
+	a := entryAllocPool.Get().(*entryAllocCache)
+	*e = entry{}
+	a.free(e)
+	entryAllocPool.Put(a)
+}
+
+var entryAllocPool = sync.Pool{
+	New: func() interface{} {
+		return newEntryAllocCache()
+	},
+}
+
+// entryAllocCacheLimit is the maximum number of entries that are cached inside
+// a pooled object.
+const entryAllocCacheLimit = 128
+
+type entryAllocCache struct {
+	entries []*entry
+}
+
+func newEntryAllocCache() *entryAllocCache {
+	c := &entryAllocCache{}
+	if !entriesGoAllocated {
+		// Note the use of a "real" finalizer here (as opposed to a build tag-gated
+		// no-op finalizer). Without the finalizer, objects released from the pool
+		// and subsequently GC'd by the Go runtime would fail to have their manually
+		// allocated memory freed, which results in a memory leak.
+		// lint:ignore SetFinalizer
+		runtime.SetFinalizer(c, freeEntryAllocCache)
+	}
+	return c
+}
+
+func freeEntryAllocCache(obj interface{}) {
+	c := obj.(*entryAllocCache)
+	for i, e := range c.entries {
+		c.dealloc(e)
+		c.entries[i] = nil
+	}
+}
+
+func (c *entryAllocCache) alloc() *entry {
+	n := len(c.entries)
+	if n == 0 {
+		if entriesGoAllocated {
+			return &entry{}
+		}
+		b := manual.New(manual.BlockCacheEntry, entrySize)
+		return (*entry)(unsafe.Pointer(&b[0]))
+	}
+	e := c.entries[n-1]
+	c.entries = c.entries[:n-1]
+	return e
+}
+
+func (c *entryAllocCache) dealloc(e *entry) {
+	if !entriesGoAllocated {
+		buf := unsafe.Slice((*byte)(unsafe.Pointer(e)), entrySize)
+		manual.Free(manual.BlockCacheEntry, buf)
+	}
+}
+
+func (c *entryAllocCache) free(e *entry) {
+	if len(c.entries) == entryAllocCacheLimit {
+		c.dealloc(e)
+		return
+	}
+	c.entries = append(c.entries, e)
+}
diff --git a/internal/cache/entry_invariants.go b/internal/cache/entry_invariants.go
deleted file mode 100644
index bd326ed7..00000000
--- a/internal/cache/entry_invariants.go
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-//
-//go:build (invariants && !race) || (tracing && !race)
-// +build invariants,!race tracing,!race
-
-package cache
-
-import (
-	"fmt"
-	"os"
-
-	"github.com/cockroachdb/pebble/internal/invariants"
-)
-
-// When the "invariants" or "tracing" build tags are enabled, we need to
-// allocate entries using the Go allocator so entry.val properly maintains a
-// reference to the Value.
-const entriesGoAllocated = true
-
-func entryAllocNew() *entry {
-	e := &entry{}
-	// Note: this is a no-op if invariants and tracing are disabled or race is
-	// enabled.
-	invariants.SetFinalizer(e, func(obj interface{}) {
-		e := obj.(*entry)
-		if *e != (entry{}) {
-			fmt.Fprintf(os.Stderr, "%p: entry was not freed", e)
-			os.Exit(1)
-		}
-	})
-	return e
-}
-
-func entryAllocFree(e *entry) {
-	*e = entry{}
-}
diff --git a/internal/cache/entry_normal.go b/internal/cache/entry_normal.go
deleted file mode 100644
index 2fab844f..00000000
--- a/internal/cache/entry_normal.go
+++ /dev/null
@@ -1,104 +0,0 @@
-// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-//
-//go:build (!invariants && !tracing) || race
-// +build !invariants,!tracing race
-
-package cache
-
-import (
-	"runtime"
-	"sync"
-	"unsafe"
-
-	"github.com/cockroachdb/pebble/internal/invariants"
-	"github.com/cockroachdb/pebble/internal/manual"
-)
-
-const (
-	entrySize            = int(unsafe.Sizeof(entry{}))
-	entryAllocCacheLimit = 128
-	// Avoid using runtime.SetFinalizer in race builds as finalizers tickle a bug
-	// in the Go race detector in go1.15 and earlier versions. This requires that
-	// entries are Go allocated rather than manually allocated.
-	//
-	// If cgo is disabled we need to allocate the entries using the Go allocator
-	// and is violates the Go GC rules to put Go pointers (such as the entry
-	// pointer fields) into untyped memory (i.e. a []byte).
-	entriesGoAllocated = invariants.RaceEnabled || !cgoEnabled
-)
-
-var entryAllocPool = sync.Pool{
-	New: func() interface{} {
-		return newEntryAllocCache()
-	},
-}
-
-func entryAllocNew() *entry {
-	a := entryAllocPool.Get().(*entryAllocCache)
-	e := a.alloc()
-	entryAllocPool.Put(a)
-	return e
-}
-
-func entryAllocFree(e *entry) {
-	a := entryAllocPool.Get().(*entryAllocCache)
-	*e = entry{}
-	a.free(e)
-	entryAllocPool.Put(a)
-}
-
-type entryAllocCache struct {
-	entries []*entry
-}
-
-func newEntryAllocCache() *entryAllocCache {
-	c := &entryAllocCache{}
-	if !entriesGoAllocated {
-		// Note the use of a "real" finalizer here (as opposed to a build tag-gated
-		// no-op finalizer). Without the finalizer, objects released from the pool
-		// and subsequently GC'd by the Go runtime would fail to have their manually
-		// allocated memory freed, which results in a memory leak.
-		// lint:ignore SetFinalizer
-		runtime.SetFinalizer(c, freeEntryAllocCache)
-	}
-	return c
-}
-
-func freeEntryAllocCache(obj interface{}) {
-	c := obj.(*entryAllocCache)
-	for i, e := range c.entries {
-		c.dealloc(e)
-		c.entries[i] = nil
-	}
-}
-
-func (c *entryAllocCache) alloc() *entry {
-	n := len(c.entries)
-	if n == 0 {
-		if entriesGoAllocated {
-			return &entry{}
-		}
-		b := manual.New(manual.BlockCacheEntry, entrySize)
-		return (*entry)(unsafe.Pointer(&b[0]))
-	}
-	e := c.entries[n-1]
-	c.entries = c.entries[:n-1]
-	return e
-}
-
-func (c *entryAllocCache) dealloc(e *entry) {
-	if !entriesGoAllocated {
-		buf := (*[manual.MaxArrayLen]byte)(unsafe.Pointer(e))[:entrySize:entrySize]
-		manual.Free(manual.BlockCacheEntry, buf)
-	}
-}
-
-func (c *entryAllocCache) free(e *entry) {
-	if len(c.entries) == entryAllocCacheLimit {
-		c.dealloc(e)
-		return
-	}
-	c.entries = append(c.entries, e)
-}
diff --git a/internal/cache/value.go b/internal/cache/value.go
index 0996e89f..c477021a 100644
--- a/internal/cache/value.go
+++ b/internal/cache/value.go
@@ -4,7 +4,15 @@
 
 package cache
 
-import "unsafe"
+import (
+	"fmt"
+	"os"
+	"unsafe"
+
+	"github.com/cockroachdb/pebble/internal/buildtags"
+	"github.com/cockroachdb/pebble/internal/invariants"
+	"github.com/cockroachdb/pebble/internal/manual"
+)
 
 // ValueMetadataSize denotes the number of bytes of metadata allocated for a
 // cache entry. Note that builds with cgo disabled allocate no metadata, and
@@ -18,12 +26,71 @@ var _ uint = ValueMetadataSize - uint(unsafe.Sizeof(Value{}))
 
 // Value holds a reference counted immutable value.
 type Value struct {
+	// buf is allocated using the manual package.
 	buf []byte
 	// Reference count for the value. The value is freed when the reference count
 	// drops to zero.
 	ref refcnt
 }
 
+// The Value struct is normally allocated together with the buffer using manual
+// memory.
+//
+// If cgo is not available, the Value must be a normal Go object to keep
+// the buffer reference visible to the GC. We also use the Go allocator if we
+// want to add finalizer assertions.
+const valueEntryGoAllocated = !buildtags.Cgo || invariants.UseFinalizers
+
+func newValue(n int) *Value {
+	if n == 0 {
+		return nil
+	}
+
+	if valueEntryGoAllocated {
+		// Note: if cgo is not enabled, manual.New will do a regular Go allocation.
+		b := manual.New(manual.BlockCacheData, n)
+		v := &Value{buf: b}
+		v.ref.init(1)
+		// Note: this is a no-op if invariants and tracing are disabled or race is
+		// enabled.
+		invariants.SetFinalizer(v, func(obj interface{}) {
+			v := obj.(*Value)
+			if v.buf != nil {
+				fmt.Fprintf(os.Stderr, "%p: cache value was not freed: refs=%d\n%s",
+					v, v.refs(), v.ref.traces())
+				os.Exit(1)
+			}
+		})
+		return v
+	}
+	// When we're not performing leak detection, the lifetime of the returned
+	// Value is exactly the lifetime of the backing buffer and we can manually
+	// allocate both.
+	b := manual.New(manual.BlockCacheData, ValueMetadataSize+n)
+	v := (*Value)(unsafe.Pointer(&b[0]))
+	v.buf = b[ValueMetadataSize:]
+	v.ref.init(1)
+	return v
+}
+
+func (v *Value) free() {
+	if invariants.Enabled {
+		// Poison the contents to help catch use-after-free bugs.
+		for i := range v.buf {
+			v.buf[i] = 0xff
+		}
+	}
+	if valueEntryGoAllocated {
+		manual.Free(manual.BlockCacheData, v.buf)
+		v.buf = nil
+		return
+	}
+	n := ValueMetadataSize + cap(v.buf)
+	buf := unsafe.Slice((*byte)(unsafe.Pointer(v)), n)
+	v.buf = nil
+	manual.Free(manual.BlockCacheData, buf)
+}
+
 // Buf returns the buffer associated with the value. The contents of the buffer
 // should not be changed once the value has been added to the cache. Instead, a
 // new Value should be created and added to the cache to replace the existing
diff --git a/internal/cache/value_cgo.go b/internal/cache/value_cgo.go
deleted file mode 100644
index 3af21cbc..00000000
--- a/internal/cache/value_cgo.go
+++ /dev/null
@@ -1,39 +0,0 @@
-// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build ((!invariants && !tracing) || race) && cgo
-// +build !invariants,!tracing race
-// +build cgo
-
-package cache
-
-import (
-	"unsafe"
-
-	"github.com/cockroachdb/pebble/internal/manual"
-)
-
-func newValue(n int) *Value {
-	if n == 0 {
-		return nil
-	}
-
-	// When we're not performing leak detection, the lifetime of the returned
-	// Value is exactly the lifetime of the backing buffer and we can manually
-	// allocate both.
-	b := manual.New(manual.BlockCacheData, ValueMetadataSize+n)
-	v := (*Value)(unsafe.Pointer(&b[0]))
-	v.buf = b[ValueMetadataSize:]
-	v.ref.init(1)
-	return v
-}
-
-func (v *Value) free() {
-	// When we're not performing leak detection, the Value and buffer were
-	// allocated contiguously.
-	n := ValueMetadataSize + cap(v.buf)
-	buf := (*[manual.MaxArrayLen]byte)(unsafe.Pointer(v))[:n:n]
-	v.buf = nil
-	manual.Free(manual.BlockCacheData, buf)
-}
diff --git a/internal/cache/value_invariants.go b/internal/cache/value_invariants.go
deleted file mode 100644
index a6d1750e..00000000
--- a/internal/cache/value_invariants.go
+++ /dev/null
@@ -1,55 +0,0 @@
-// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build (invariants && !race) || (tracing && !race)
-// +build invariants,!race tracing,!race
-
-package cache
-
-import (
-	"fmt"
-	"os"
-
-	"github.com/cockroachdb/pebble/internal/invariants"
-	"github.com/cockroachdb/pebble/internal/manual"
-)
-
-// newValue creates a Value with a manually managed buffer of size n.
-//
-// This definition of newValue is used when either the "invariants" or
-// "tracing" build tags are specified. It hooks up a finalizer to the returned
-// Value that checks for memory leaks when the GC determines the Value is no
-// longer reachable.
-func newValue(n int) *Value {
-	if n == 0 {
-		return nil
-	}
-	b := manual.New(manual.BlockCacheData, n)
-	v := &Value{buf: b}
-	v.ref.init(1)
-	// Note: this is a no-op if invariants and tracing are disabled or race is
-	// enabled.
-	invariants.SetFinalizer(v, func(obj interface{}) {
-		v := obj.(*Value)
-		if v.buf != nil {
-			fmt.Fprintf(os.Stderr, "%p: cache value was not freed: refs=%d\n%s",
-				v, v.refs(), v.ref.traces())
-			os.Exit(1)
-		}
-	})
-	return v
-}
-
-func (v *Value) free() {
-	// When "invariants" are enabled set the value contents to 0xff in order to
-	// cache use-after-free bugs.
-	for i := range v.buf {
-		v.buf[i] = 0xff
-	}
-	manual.Free(manual.BlockCacheData, v.buf)
-	// Setting Value.buf to nil is needed for correctness of the leak checking
-	// that is performed when the "invariants" or "tracing" build tags are
-	// enabled.
-	v.buf = nil
-}
diff --git a/internal/cache/value_normal.go b/internal/cache/value_normal.go
deleted file mode 100644
index 1926b757..00000000
--- a/internal/cache/value_normal.go
+++ /dev/null
@@ -1,24 +0,0 @@
-// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build ((!invariants && !tracing) || race) && !cgo
-// +build !invariants,!tracing race
-// +build !cgo
-
-package cache
-
-func newValue(n int) *Value {
-	if n == 0 {
-		return nil
-	}
-
-	// Since Cgo is disabled then all memory is allocated from the Go heap we
-	// can't play the trick below to combine the Value and buffer allocation.
-	v := &Value{buf: make([]byte, n)}
-	v.ref.init(1)
-	return v
-}
-
-func (v *Value) free() {
-}
diff --git a/internal/invariants/finalizer_off.go b/internal/invariants/finalizer_off.go
deleted file mode 100644
index d2c600ae..00000000
--- a/internal/invariants/finalizer_off.go
+++ /dev/null
@@ -1,14 +0,0 @@
-// Copyright 2021 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build (!invariants && !tracing) || race
-// +build !invariants,!tracing race
-
-package invariants
-
-// SetFinalizer is a wrapper around runtime.SetFinalizer that is a no-op under
-// race builds or if neither the invariants or tracing build tags are
-// specified.
-func SetFinalizer(obj, finalizer interface{}) {
-}
diff --git a/internal/invariants/finalizer_on.go b/internal/invariants/finalizer_on.go
deleted file mode 100644
index da4e307b..00000000
--- a/internal/invariants/finalizer_on.go
+++ /dev/null
@@ -1,17 +0,0 @@
-// Copyright 2021 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-//go:build (invariants && !race) || (tracing && !race)
-// +build invariants,!race tracing,!race
-
-package invariants
-
-import "runtime"
-
-// SetFinalizer is a wrapper around runtime.SetFinalizer that is a no-op under
-// race builds or if neither the invariants or tracing build tags are
-// specified.
-func SetFinalizer(obj, finalizer interface{}) {
-	runtime.SetFinalizer(obj, finalizer)
-}
diff --git a/internal/invariants/invariants.go b/internal/invariants/invariants.go
new file mode 100644
index 00000000..2a0cee5e
--- /dev/null
+++ b/internal/invariants/invariants.go
@@ -0,0 +1,51 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+package invariants
+
+import (
+	"math/rand/v2"
+	"runtime"
+
+	"github.com/cockroachdb/pebble/internal/buildtags"
+)
+
+// Enabled is true if we were built with the "invariants" or "race" build tags.
+//
+// Enabled should be used to gate invariant checks that may be expensive. It
+// should not be used to unconditionally alter a code path significantly (e.g.
+// wrapping an iterator - see #3678); Sometimes() should be used instead so that
+// the production code path gets test coverage as well.
+const Enabled = buildtags.Race || buildtags.Invariants
+
+// RaceEnabled is true if we were built with the "race" build tag.
+const RaceEnabled = buildtags.Race
+
+// Sometimes returns true percent% of the time if invariants are Enabled (i.e.
+// we were built with the "invariants" or "race" build tags). Otherwise, always
+// returns false.
+func Sometimes(percent int) bool {
+	return Enabled && rand.Uint32N(100) < uint32(percent)
+}
+
+// UseFinalizers is true if we want to use finalizers for assertions around
+// object lifetime and cleanup. This happens when the invariants or tracing tags
+// are set, but we exclude race builds because we historically ran into some
+// finalizer-related race detector bugs.
+const UseFinalizers = !buildtags.Race && (buildtags.Invariants || buildtags.Tracing)
+
+// SetFinalizer is a wrapper around runtime.SetFinalizer that is a no-op under
+// race builds or if neither the invariants nor tracing build tags are
+// specified.
+//
+// We exclude race builds because we historically ran into some race detector
+// bugs related to finalizers.
+//
+// This function is a no-op if UseFinalizers is false and it should inline to
+// nothing.
+func SetFinalizer(obj, finalizer interface{}) {
+	if UseFinalizers {
+		runtime.SetFinalizer(obj, finalizer)
+	}
+}
diff --git a/internal/invariants/off.go b/internal/invariants/off.go
index a6b98b0c..c31c32bf 100644
--- a/internal/invariants/off.go
+++ b/internal/invariants/off.go
@@ -3,19 +3,15 @@
 // the LICENSE file.
 
 //go:build !invariants && !race
-// +build !invariants,!race
 
 package invariants
 
-// Enabled is true if we were built with the "invariants" or "race" build tags.
+// CloseChecker is used to check that objects are closed exactly once. It is
+// empty and does nothing in non-invariant builds.
 //
-// Enabled should be used to gate invariant checks that may be expensive. It
-// should not be used to unconditionally alter a code path significantly (e.g.
-// wrapping an iterator - see #3678); Sometimes() should be used instead so that
-// the production code path gets test coverage as well.
-const Enabled = false
-
-// CloseChecker is used to check that objects are closed exactly once.
+// Note that in non-invariant builds, the struct is zero-sized but it can still
+// increase the size of a parent struct if it is the last field (because Go must
+// allow getting a valid pointer address of the field).
 type CloseChecker struct{}
 
 // Close panics if called twice on the same object (if we were built with the
@@ -32,13 +28,17 @@ func (d *CloseChecker) AssertNotClosed() {}
 // builds. In non-invariant builds, storing a value is a no-op, retrieving a
 // value returns the type parameter's zero value, and the Value struct takes up
 // no space.
+//
+// Note that in non-invariant builds, the struct is zero-sized but it can still
+// increase the size of a parent struct if it is the last field (because Go must
+// allow getting a valid pointer address of the field).
 type Value[V any] struct{}
 
-// Get returns the current value, or the zero-value if invariants are disabled.
+// Get the current value, or the zero value if invariants are disabled.
 func (*Value[V]) Get() V {
 	var v V // zero value
 	return v
 }
 
-// Store stores the value.
-func (*Value[V]) Store(v V) {}
+// Set the value; no-op in non-invariant builds.
+func (*Value[V]) Set(v V) {}
diff --git a/internal/invariants/on.go b/internal/invariants/on.go
index a6ce4abf..0a09894b 100644
--- a/internal/invariants/on.go
+++ b/internal/invariants/on.go
@@ -3,14 +3,15 @@
 // the LICENSE file.
 
 //go:build invariants || race
-// +build invariants race
 
 package invariants
 
-// Enabled is true if we were built with the "invariants" or "race" build tags.
-const Enabled = true
-
-// CloseChecker is used to check that objects are closed exactly once.
+// CloseChecker is used to check that objects are closed exactly once. It is
+// empty and does nothing in non-invariant builds.
+//
+// Note that in non-invariant builds, the struct is zero-sized but it can still
+// increase the size of a parent struct if it is the last field (because Go must
+// allow getting a valid pointer address of the field).
 type CloseChecker struct {
 	closed bool
 }
@@ -45,16 +46,20 @@ func (d *CloseChecker) AssertNotClosed() {
 // builds. In non-invariant builds, storing a value is a no-op, retrieving a
 // value returns the type parameter's zero value, and the Value struct takes up
 // no space.
+//
+// Note that in non-invariant builds, the struct is zero-sized but it can still
+// increase the size of a parent struct if it is the last field (because Go must
+// allow getting a valid pointer address of the field).
 type Value[V any] struct {
 	v V
 }
 
-// Get returns the current value, or the zero-value if invariants are disabled.
+// Get the current value, or the zero-value if invariants are disabled.
 func (v *Value[V]) Get() V {
 	return v.v
 }
 
-// Store stores the value.
-func (v *Value[V]) Store(inner V) {
+// Set the value; no-op in non-invariant builds.
+func (v *Value[V]) Set(inner V) {
 	v.v = inner
 }
diff --git a/internal/invariants/utils.go b/internal/invariants/utils.go
deleted file mode 100644
index 9faef95f..00000000
--- a/internal/invariants/utils.go
+++ /dev/null
@@ -1,13 +0,0 @@
-// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
-// of this source code is governed by a BSD-style license that can be found in
-// the LICENSE file.
-
-package invariants
-
-import "math/rand/v2"
-
-// Sometimes returns true percent% of the time if we were built with the
-// "invariants" of "race" build tags
-func Sometimes(percent int) bool {
-	return Enabled && rand.Uint32N(100) < uint32(percent)
-}
diff --git a/options.go b/options.go
index cd821562..e2bf1013 100644
--- a/options.go
+++ b/options.go
@@ -14,8 +14,8 @@ import (
 	"time"
 	"unicode"
 
+	"github.com/cockroachdb/crlib/fifo"
 	"github.com/cockroachdb/errors"
-	"github.com/cockroachdb/fifo"
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/cache"
 	"github.com/cockroachdb/pebble/internal/humanize"
diff --git a/sstable/colblk_writer.go b/sstable/colblk_writer.go
index eae0e92c..c1df666c 100644
--- a/sstable/colblk_writer.go
+++ b/sstable/colblk_writer.go
@@ -477,7 +477,7 @@ func (w *RawColumnWriter) evaluatePoint(
 	// When invariants are enabled, validate kcmp.
 	if invariants.Enabled {
 		colblk.AssertKeyCompare(w.comparer, key.UserKey, w.previousUserKey.Get(), eval.kcmp)
-		w.previousUserKey.Store(append(w.previousUserKey.Get()[:0], key.UserKey...))
+		w.previousUserKey.Set(append(w.previousUserKey.Get()[:0], key.UserKey...))
 	}
 
 	if !w.meta.HasPointKeys {
diff --git a/sstable/options.go b/sstable/options.go
index ac06af98..be5a689c 100644
--- a/sstable/options.go
+++ b/sstable/options.go
@@ -7,7 +7,7 @@ package sstable
 import (
 	"fmt"
 
-	"github.com/cockroachdb/fifo"
+	"github.com/cockroachdb/crlib/fifo"
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/sstable/block"
diff --git a/sstable/reader.go b/sstable/reader.go
index 093b3186..52dd7926 100644
--- a/sstable/reader.go
+++ b/sstable/reader.go
@@ -17,8 +17,8 @@ import (
 
 	"github.com/cespare/xxhash/v2"
 	"github.com/cockroachdb/crlib/crtime"
+	"github.com/cockroachdb/crlib/fifo"
 	"github.com/cockroachdb/errors"
-	"github.com/cockroachdb/fifo"
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/bytealloc"
 	"github.com/cockroachdb/pebble/internal/cache"
diff --git a/wal/failover_manager.go b/wal/failover_manager.go
index f9bcba06..ae474858 100644
--- a/wal/failover_manager.go
+++ b/wal/failover_manager.go
@@ -530,7 +530,7 @@ func (wm *failoverManager) init(o Options, initial Logs) error {
 }
 
 // List implements Manager.
-func (wm *failoverManager) List() (Logs, error) {
+func (wm *failoverManager) List() Logs {
 	wm.mu.Lock()
 	defer wm.mu.Unlock()
 	n := len(wm.mu.closedWALs)
@@ -554,7 +554,7 @@ func (wm *failoverManager) List() (Logs, error) {
 	if wm.mu.ww != nil {
 		setLogicalLog(n-1, wm.mu.ww.getLog())
 	}
-	return wals, nil
+	return wals
 }
 
 // Obsolete implements Manager.
diff --git a/wal/standalone_manager.go b/wal/standalone_manager.go
index 26dae5d5..2c228369 100644
--- a/wal/standalone_manager.go
+++ b/wal/standalone_manager.go
@@ -78,7 +78,7 @@ func (m *StandaloneManager) init(o Options, initial Logs) error {
 }
 
 // List implements Manager.
-func (m *StandaloneManager) List() (Logs, error) {
+func (m *StandaloneManager) List() Logs {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	wals := make(Logs, len(m.mu.queue))
@@ -88,7 +88,7 @@ func (m *StandaloneManager) List() (Logs, error) {
 			segments: []segment{{dir: m.o.Primary}},
 		}
 	}
-	return wals, nil
+	return wals
 }
 
 // Obsolete implements Manager.
diff --git a/wal/wal.go b/wal/wal.go
index d97bedd5..2616442c 100644
--- a/wal/wal.go
+++ b/wal/wal.go
@@ -323,8 +323,9 @@ type Manager interface {
 	// init initializes the Manager. init is called during DB initialization.
 	init(o Options, initial Logs) error
 
-	// List returns the virtual WALs in ascending order.
-	List() (Logs, error)
+	// List returns the virtual WALs in ascending order. List must not perform
+	// I/O.
+	List() Logs
 	// Obsolete informs the manager that all virtual WALs less than
 	// minUnflushedNum are obsolete. The callee can choose to recycle some
 	// underlying log files, if !noRecycle. The log files that are not recycled,


Test patch:
diff --git a/db_test.go b/db_test.go
index fee5482a..5bd26776 100644
--- a/db_test.go
+++ b/db_test.go
@@ -21,9 +21,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cockroachdb/crlib/fifo"
 	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/errors"
-	"github.com/cockroachdb/fifo"
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/cache"
 	"github.com/cockroachdb/pebble/internal/testkeys"
@@ -383,8 +383,7 @@ func TestLargeBatch(t *testing.T) {
 	getLatestLog := func() wal.LogicalLog {
 		d.mu.Lock()
 		defer d.mu.Unlock()
-		logs, err := d.mu.log.manager.List()
-		require.NoError(t, err)
+		logs := d.mu.log.manager.List()
 		return logs[len(logs)-1]
 	}
 	memTableCreationSeqNum := func() base.SeqNum {
@@ -1996,15 +1995,13 @@ func TestRecycleLogs(t *testing.T) {
 	logNum := func() base.DiskFileNum {
 		d.mu.Lock()
 		defer d.mu.Unlock()
-		walNums, err := d.mu.log.manager.List()
-		require.NoError(t, err)
+		walNums := d.mu.log.manager.List()
 		return base.DiskFileNum(walNums[len(walNums)-1].Num)
 	}
 	logCount := func() int {
 		d.mu.Lock()
 		defer d.mu.Unlock()
-		walNums, err := d.mu.log.manager.List()
-		require.NoError(t, err)
+		walNums := d.mu.log.manager.List()
 		return len(walNums)
 	}
 
diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go
index 8d430ed2..0e8ba78c 100644
--- a/internal/lint/lint_test.go
+++ b/internal/lint/lint_test.go
@@ -187,7 +187,7 @@ func TestLint(t *testing.T) {
 			stream.Sequence(
 				dirCmd(t, pkg.Dir, "git", "grep", "-B1", "runtime\\.SetFinalizer("),
 				lintIgnore("lint:ignore SetFinalizer"),
-				stream.GrepNot(`^internal/invariants/finalizer_on.go`),
+				stream.GrepNot(`^internal/invariants/invariants.go`),
 			), func(s string) {
 				t.Errorf("\n%s <- please use the \"invariants.SetFinalizer\" equivalent instead", s)
 			}); err != nil {
diff --git a/wal/failover_manager_test.go b/wal/failover_manager_test.go
index 770af4a9..cc6635e5 100644
--- a/wal/failover_manager_test.go
+++ b/wal/failover_manager_test.go
@@ -424,10 +424,7 @@ func TestManagerFailover(t *testing.T) {
 				return b.String()
 
 			case "list-and-stats":
-				logs, err := fm.List()
-				if err != nil {
-					return err.Error()
-				}
+				logs := fm.List()
 				stats := fm.Stats()
 				var b strings.Builder
 				if len(logs) > 0 {