From d542aa7bbbf07e0b47a1e206d23654d980962f06 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Thu, 14 Nov 2024 05:54:33 -0800 Subject: [PATCH] cache: simplify entry code Consolidate the entry allocation paths using constants instead of build tags. --- internal/cache/cgo_disabled.go | 10 --- internal/cache/cgo_enabled.go | 10 --- internal/cache/entry.go | 123 ++++++++++++++++++++++++++++- internal/cache/entry_invariants.go | 38 --------- internal/cache/entry_normal.go | 104 ------------------------ internal/cache/value.go | 1 - internal/cache/value_cgo.go | 40 ---------- internal/cache/value_invariants.go | 55 ------------- internal/cache/value_normal.go | 25 ------ 9 files changed, 122 insertions(+), 284 deletions(-) delete mode 100644 internal/cache/cgo_disabled.go delete mode 100644 internal/cache/cgo_enabled.go delete mode 100644 internal/cache/entry_invariants.go delete mode 100644 internal/cache/entry_normal.go delete mode 100644 internal/cache/value_cgo.go delete mode 100644 internal/cache/value_invariants.go delete mode 100644 internal/cache/value_normal.go diff --git a/internal/cache/cgo_disabled.go b/internal/cache/cgo_disabled.go deleted file mode 100644 index 0e7557455a..0000000000 --- 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 b7014cb06b..0000000000 --- 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 5473a86704..724cb29017 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 bd326ed79e..0000000000 --- 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 2fab844f4d..0000000000 --- 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 8e20320e59..c477021aaa 100644 --- a/internal/cache/value.go +++ b/internal/cache/value.go @@ -63,7 +63,6 @@ func newValue(n int) *Value { }) 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. diff --git a/internal/cache/value_cgo.go b/internal/cache/value_cgo.go deleted file mode 100644 index 3268d63f8c..0000000000 --- a/internal/cache/value_cgo.go +++ /dev/null @@ -1,40 +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 && foooo -// +build !invariants,!tracing race -// +build cgo -// +build foooo - -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 0ee6996a58..0000000000 --- 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 && foooo) || (tracing && !race && fooo) -// +build invariants,!race,foooo tracing,!race,fooo - -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 b8d57a20e0..0000000000 --- a/internal/cache/value_normal.go +++ /dev/null @@ -1,25 +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 && fooooo -// +build !invariants,!tracing race -// +build !cgo -// +build fooooo - -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() { -}