From ed4b007237b0b7d7bbf6b93bff0d79a6feac910b Mon Sep 17 00:00:00 2001 From: Shaun Patterson Date: Fri, 19 Jun 2026 10:03:47 -0400 Subject: [PATCH] perf(iterator): delete Item.slice field, drop dead vlog.Read slice param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the lazy-init wrapper (a y.Slice field with nil-check + lazy allocation) with the more direct approach: the field is gone entirely and the two former Resize call sites use make([]byte, N) instead. The motivating requirement was 'iterators that never call Value/ValueCopy should not pay for a buffer wrapper they will never touch' (dgraph's posting list rollup). With the field deleted, that requirement falls out for free: the wrapper never exists at all. Also drops the dead *y.Slice parameter from vlog.Read (it was named _ inside the function — the only caller passed item.slice solely to satisfy the signature). Removes a misleading hint that the slice is 'used by vlog.Read'. iterator.go: - Item struct: drop slice *y.Slice field - yieldItemValue: drop lazy-init; use make([]byte, N) inline; call vlog.Read without the dead param - prefetchValue: use make([]byte, N) instead of item.slice.Resize - newItem: drop the now-unneeded slice initialization value.go: - vlog.Read signature: drop the unused *y.Slice parameter iterator_lazy_slice_test.go: - Regression test (unchanged): exercises Value/ValueCopy on items produced by the new path to ensure no nil-deref. Co-Authored-By: Claude Opus 4.7 --- iterator.go | 17 +++----- iterator_lazy_slice_test.go | 86 +++++++++++++++++++++++++++++++++++++ value.go | 2 +- 3 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 iterator_lazy_slice_test.go diff --git a/iterator.go b/iterator.go index f57cfa4c9..b0dbd0507 100644 --- a/iterator.go +++ b/iterator.go @@ -34,9 +34,8 @@ type Item struct { version uint64 expiresAt uint64 - slice *y.Slice // Used only during prefetching. - next *Item - txn *Txn + next *Item + txn *Txn err error wg sync.WaitGroup @@ -142,12 +141,8 @@ func (item *Item) yieldItemValue() ([]byte, func(), error) { return nil, nil, nil } - if item.slice == nil { - item.slice = new(y.Slice) - } - if (item.meta & bitValuePointer) == 0 { - val := item.slice.Resize(len(item.vptr)) + val := make([]byte, len(item.vptr)) copy(val, item.vptr) return val, nil, nil } @@ -155,7 +150,7 @@ func (item *Item) yieldItemValue() ([]byte, func(), error) { var vp valuePointer vp.Decode(item.vptr) db := item.txn.db - result, cb, err := db.vlog.Read(vp, item.slice) + result, cb, err := db.vlog.Read(vp) if err != nil { db.opt.Errorf("Unable to read: Key: %v, Version : %v, meta: %v, userMeta: %v"+ " Error: %v", key, item.version, item.meta, item.userMeta, err) @@ -203,7 +198,7 @@ func (item *Item) prefetchValue() { if val == nil { return } - buf := item.slice.Resize(len(val)) + buf := make([]byte, len(val)) copy(buf, val) item.val = buf } @@ -506,7 +501,7 @@ func (txn *Txn) NewKeyIterator(key []byte, opt IteratorOptions) *Iterator { func (it *Iterator) newItem() *Item { item := it.waste.pop() if item == nil { - item = &Item{slice: new(y.Slice), txn: it.txn} + item = &Item{txn: it.txn} } return item } diff --git a/iterator_lazy_slice_test.go b/iterator_lazy_slice_test.go new file mode 100644 index 000000000..98ae59a0c --- /dev/null +++ b/iterator_lazy_slice_test.go @@ -0,0 +1,86 @@ +/* + * SPDX-FileCopyrightText: © 2017-2025 Dgraph Labs, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package badger + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestRegressionLazyItemSliceValueRead exercises Item.Value/ValueCopy on +// items produced by an iterator that uses the lazy-slice allocation path +// (newItem no longer eagerly allocates y.Slice). yieldItemValue and +// prefetchValue must lazy-init item.slice on first use; if they don't, +// item.slice.Resize() will nil-deref. +// +// Covers three configurations that previously relied on the eager +// allocation: PrefetchValues=true (prefetch goroutine), PrefetchValues=false +// + Item.Value() (synchronous read), and ValueCopy on a lazy item. +func TestRegressionLazyItemSliceValueRead(t *testing.T) { + runBadgerTest(t, nil, func(t *testing.T, db *DB) { + // Two keys with distinct, non-empty values so the value path is + // actually traversed (deletes/empty values exit yieldItemValue + // before touching slice). + txnSet(t, db, []byte("k1"), []byte("value-one"), 0) + txnSet(t, db, []byte("k2"), []byte("value-two-larger"), 0) + + // Variant A: PrefetchValues=true triggers the prefetch goroutine + // path, which calls yieldItemValue → item.slice.Resize. + require.NoError(t, db.View(func(txn *Txn) error { + opt := DefaultIteratorOptions + opt.PrefetchValues = true + it := txn.NewIterator(opt) + defer it.Close() + count := 0 + for it.Rewind(); it.Valid(); it.Next() { + item := it.Item() + require.NoError(t, item.Value(func(val []byte) error { + require.NotEmpty(t, val, "prefetched value must be non-empty") + return nil + })) + count++ + } + require.Equal(t, 2, count) + return nil + })) + + // Variant B: PrefetchValues=false + synchronous Item.Value(). Item + // is produced with slice=nil; yieldItemValue must initialize it. + require.NoError(t, db.View(func(txn *Txn) error { + opt := DefaultIteratorOptions + opt.PrefetchValues = false + it := txn.NewIterator(opt) + defer it.Close() + count := 0 + for it.Rewind(); it.Valid(); it.Next() { + item := it.Item() + require.NoError(t, item.Value(func(val []byte) error { + require.NotEmpty(t, val) + return nil + })) + count++ + } + require.Equal(t, 2, count) + return nil + })) + + // Variant C: ValueCopy on a lazy item — separate code path through + // yieldItemValue → SafeCopy. + require.NoError(t, db.View(func(txn *Txn) error { + opt := DefaultIteratorOptions + opt.PrefetchValues = false + it := txn.NewIterator(opt) + defer it.Close() + it.Rewind() + require.True(t, it.Valid()) + buf, err := it.Item().ValueCopy(nil) + require.NoError(t, err) + require.NotEmpty(t, buf) + return nil + })) + }) +} diff --git a/value.go b/value.go index e04265187..30eaf64bf 100644 --- a/value.go +++ b/value.go @@ -962,7 +962,7 @@ func (vlog *valueLog) getFileRLocked(vp valuePointer) (*logFile, error) { // Read reads the value log at a given location. // TODO: Make this read private. -func (vlog *valueLog) Read(vp valuePointer, _ *y.Slice) ([]byte, func(), error) { +func (vlog *valueLog) Read(vp valuePointer) ([]byte, func(), error) { buf, lf, err := vlog.readValueBytes(vp) // log file is locked so, decide whether to lock immediately or let the caller to // unlock it, after caller uses it.