Repository: cockroachdb/pebble
Pull request #1757 (2022-06-11 00:03:45)

db: avoid allocation in newIters
Avoid allocation in newIters by using the cached closeHook closure. This
optimization was lost in 09203fd9a780ce8c15bf8bec3c773ff028051e02. Some version
of this should be backported to 22.1.

```
name                                                    old time/op    new time/op    delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8-10      4.40µs ±12%    3.76µs ± 1%  -14.42%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8-10     5.01µs ± 3%    4.49µs ± 1%  -10.32%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8-10    9.56µs ± 2%    9.32µs ± 2%   -2.55%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8-10       2.19µs ± 1%    2.15µs ± 0%   -1.71%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8-10      3.06µs ± 2%    2.97µs ± 0%   -3.05%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8-10     6.77µs ± 2%    6.50µs ± 1%   -3.93%  (p=0.000 n=9+9)

name                                                    old speed      new speed      delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8-10    1.83MB/s ±11%  2.13MB/s ± 1%  +16.40%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8-10   1.60MB/s ± 3%  1.78MB/s ± 1%  +11.51%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8-10   837kB/s ± 2%   859kB/s ± 2%   +2.62%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8-10     3.66MB/s ± 1%  3.72MB/s ± 0%   +1.70%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8-10    2.62MB/s ± 2%  2.70MB/s ± 0%   +3.17%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8-10   1.18MB/s ± 2%  1.23MB/s ± 0%   +4.04%  (p=0.000 n=9+6)

name                                                    old allocs/op  new allocs/op  delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8-10        4.00 ± 0%      2.00 ± 0%  -50.00%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8-10       4.00 ± 0%      2.00 ± 0%  -50.00%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8-10      6.00 ± 0%      2.00 ± 0%  -66.67%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8-10         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8-10        2.00 ± 0%      2.00 ± 0%     ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8-10       3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
```

cockroach-teamcity: 

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


Patch:
diff --git a/db.go b/db.go
index 320d828b..d9643fa3 100644
--- a/db.go
+++ b/db.go
@@ -473,11 +473,6 @@ type DB struct {
 		}
 	}
 
-	// rangeKeys is a temporary field so that Pebble can provide a non-durable
-	// implementation of range keys in advance of the real implementation.
-	// TODO(jackson): Remove this.
-	rangeKeys *RangeKeysArena
-
 	// Normally equal to time.Now() but may be overridden in tests.
 	timeNow func() time.Time
 }
@@ -747,9 +742,6 @@ func (d *DB) Apply(batch *Batch, opts *WriteOptions) error {
 		if d.split == nil {
 			return errNoSplit
 		}
-		if d.opts.Experimental.RangeKeys == nil {
-			panic("pebble: range keys require the Experimental.RangeKeys option")
-		}
 		if d.FormatMajorVersion() < FormatRangeKeys {
 			panic(fmt.Sprintf(
 				"pebble: range keys require at least format major version %d (current: %d)",
@@ -893,9 +885,6 @@ func (d *DB) newIterInternal(batch *Batch, s *Snapshot, o *IterOptions) *Iterato
 		panic(err)
 	}
 	if o.rangeKeys() {
-		if d.opts.Experimental.RangeKeys == nil {
-			panic("pebble: range keys require the Experimental.RangeKeys option")
-		}
 		if d.FormatMajorVersion() < FormatRangeKeys {
 			panic(fmt.Sprintf(
 				"pebble: range keys require at least format major version %d (current: %d)",
diff --git a/open.go b/open.go
index 31d427e4..4010a55f 100644
--- a/open.go
+++ b/open.go
@@ -75,7 +75,6 @@ func Open(dirname string, opts *Options) (db *DB, _ error) {
 		merge:               opts.Merger.Merge,
 		split:               opts.Comparer.Split,
 		abbreviatedKey:      opts.Comparer.AbbreviatedKey,
-		rangeKeys:           opts.Experimental.RangeKeys,
 		largeBatchThreshold: (opts.MemTableSize - int(memTableEmptySize)) / 2,
 		logRecycler:         logRecycler{limit: opts.MemTableStopWritesThreshold + 1},
 		closed:              new(atomic.Value),
diff --git a/options.go b/options.go
index e2a3946c..d0d23887 100644
--- a/options.go
+++ b/options.go
@@ -446,12 +446,6 @@ type Options struct {
 		// deletion pacing, which is also the default.
 		MinDeletionRate int
 
-		// RangeKeys enables the experimental use of range keys, stored in an
-		// in-memory nondurable arena. This option is only intended to be
-		// temporary, to allow the Pebble metamorphic tests to reuse the
-		// range-key arena across DB restarts.
-		RangeKeys *RangeKeysArena
-
 		// ReadCompactionRate controls the frequency of read triggered
 		// compactions by adjusting `AllowedSeeks` in manifest.FileMetadata:
 		//
diff --git a/range_keys.go b/range_keys.go
index dfe5b57e..4ad233db 100644
--- a/range_keys.go
+++ b/range_keys.go
@@ -5,50 +5,13 @@
 package pebble
 
 import (
-	"sync"
-
-	"github.com/cockroachdb/pebble/internal/arenaskl"
 	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/manifest"
-	"github.com/cockroachdb/pebble/internal/rangekey"
 )
 
-const rangeKeyArenaSize = 1 << 20
-
-// RangeKeysArena is an in-memory arena in which range keys are stored.
-//
-// This is a temporary type that will eventually be removed.
-//
-// TODO(bilal): This type should mostly be unused now. Clean up the last few
-// uses and remove it.
-type RangeKeysArena struct {
-	once      sync.Once
-	skl       arenaskl.Skiplist
-	arena     *arenaskl.Arena
-	fragCache keySpanCache
-}
-
-func (d *DB) maybeInitializeRangeKeys() {
-	// Lazily construct the global range key arena, so that tests that
-	// don't use range keys don't need to allocate this long-lived
-	// buffer.
-	d.rangeKeys.once.Do(func() {
-		arenaBuf := make([]byte, rangeKeyArenaSize)
-		d.rangeKeys.arena = arenaskl.NewArena(arenaBuf)
-		d.rangeKeys.skl.Reset(d.rangeKeys.arena, d.cmp)
-		d.rangeKeys.fragCache = keySpanCache{
-			cmp:           d.cmp,
-			formatKey:     d.opts.Comparer.FormatKey,
-			skl:           &d.rangeKeys.skl,
-			constructSpan: rangekey.Decode,
-		}
-	})
-}
-
 func (d *DB) newRangeKeyIter(
 	it *Iterator, seqNum, batchSeqNum uint64, batch *Batch, readState *readState,
 ) keyspan.FragmentIterator {
-	d.maybeInitializeRangeKeys()
 	it.rangeKey.rangeKeyIter = it.rangeKey.iterConfig.Init(d.cmp, seqNum)
 
 	// If there's an indexed batch with range keys, include it.
diff --git a/sstable/reader.go b/sstable/reader.go
index fd04b0a8..13d8ac30 100644
--- a/sstable/reader.go
+++ b/sstable/reader.go
@@ -93,14 +93,6 @@ type Iterator interface {
 	SetCloseHook(fn func(i Iterator) error)
 }
 
-// FragmentIterator is a version of Iterator that wraps keyspan.FragmentIterator
-// instead of InternalIterator, and adds SetCloseHook.
-type FragmentIterator interface {
-	keyspan.FragmentIterator
-
-	SetCloseHook(fn func(i keyspan.FragmentIterator) error)
-}
-
 // singleLevelIterator iterates over an entire table of data. To seek for a given
 // key, it first looks in the index for the block that contains that key, and then
 // looks inside that block.
@@ -2218,7 +2210,7 @@ func (r *Reader) NewRawRangeDelIter() (keyspan.FragmentIterator, error) {
 // NewRawRangeKeyIter returns an internal iterator for the contents of the
 // range-key block for the table. Returns nil if the table does not contain any
 // range keys.
-func (r *Reader) NewRawRangeKeyIter() (FragmentIterator, error) {
+func (r *Reader) NewRawRangeKeyIter() (keyspan.FragmentIterator, error) {
 	if r.rangeKeyBH.Length == 0 {
 		return nil, nil
 	}
diff --git a/table_cache.go b/table_cache.go
index c5f53e41..e792152e 100644
--- a/table_cache.go
+++ b/table_cache.go
@@ -378,10 +378,9 @@ func (c *tableCacheShard) newIters(
 		c.unrefValue(v)
 		return nil, nil, err
 	}
-	// NB: v.closeHook takes responsibility for calling unrefValue(v) here.
-	iter.SetCloseHook(func(i sstable.Iterator) error {
-		return v.closeHook(i)
-	})
+	// NB: v.closeHook takes responsibility for calling unrefValue(v) here. Take
+	// care to avoid introduceingan allocation here by adding a closure.
+	iter.SetCloseHook(v.closeHook)
 
 	atomic.AddInt32(&c.atomic.iterCount, 1)
 	atomic.AddInt32(dbOpts.atomic.iterCount, 1)
@@ -435,7 +434,7 @@ func (c *tableCacheShard) newRangeKeyIter(
 		return emptyKeyspanIter, err
 	}
 
-	var iter sstable.FragmentIterator
+	var iter keyspan.FragmentIterator
 	iter, err = v.reader.NewRawRangeKeyIter()
 	// iter is a block iter that holds the entire value of the block in memory.
 	// No need to hold onto a ref of the cache value.
@@ -594,7 +593,7 @@ func (c *tableCacheShard) findNode(meta *fileMetadata, dbOpts *tableCacheOpts) *
 	}
 	// Cache the closure invoked when an iterator is closed. This avoids an
 	// allocation on every call to newIters.
-	v.closeHook = func(i io.Closer) error {
+	v.closeHook = func(i sstable.Iterator) error {
 		if invariants.RaceEnabled {
 			c.mu.Lock()
 			delete(c.mu.iters, i)
@@ -824,7 +823,7 @@ func (c *tableCacheShard) Close() error {
 }
 
 type tableCacheValue struct {
-	closeHook func(i io.Closer) error
+	closeHook func(i sstable.Iterator) error
 	reader    *sstable.Reader
 	filename  string
 	err       error


Test patch:
diff --git a/batch_test.go b/batch_test.go
index 45c0d70f..6c79b3c1 100644
--- a/batch_test.go
+++ b/batch_test.go
@@ -346,7 +346,6 @@ func TestIndexedBatchMutation(t *testing.T) {
 		FS:                 vfs.NewMem(),
 		FormatMajorVersion: FormatNewest,
 	}
-	opts.Experimental.RangeKeys = new(RangeKeysArena)
 	d, err := Open("", opts)
 	require.NoError(t, err)
 	defer func() { d.Close() }()
@@ -436,7 +435,6 @@ func TestIndexedBatch_GlobalVisibility(t *testing.T) {
 		FormatMajorVersion: FormatNewest,
 		Comparer:           testkeys.Comparer,
 	}
-	opts.Experimental.RangeKeys = new(RangeKeysArena)
 	d, err := Open("", opts)
 	require.NoError(t, err)
 	defer d.Close()
@@ -1185,7 +1183,6 @@ func TestBatchSpanCaching(t *testing.T) {
 		FS:                 vfs.NewMem(),
 		FormatMajorVersion: FormatNewest,
 	}
-	opts.Experimental.RangeKeys = new(RangeKeysArena)
 	d, err := Open("", opts)
 	require.NoError(t, err)
 	defer d.Close()
diff --git a/compaction_test.go b/compaction_test.go
index 6c734633..119b2f30 100644
--- a/compaction_test.go
+++ b/compaction_test.go
@@ -1347,13 +1347,10 @@ func TestManualCompaction(t *testing.T) {
 
 				mem = vfs.NewMem()
 				opts := &Options{
-					FS:                 mem,
-					DebugCheck:         DebugCheckLevels,
-					FormatMajorVersion: randVersion(minVersion, maxVersion),
-				}
-				opts.DisableAutomaticCompactions = true
-				if opts.FormatMajorVersion >= FormatRangeKeys {
-					opts.Experimental.RangeKeys = new(RangeKeysArena)
+					FS:                          mem,
+					DebugCheck:                  DebugCheckLevels,
+					FormatMajorVersion:          randVersion(minVersion, maxVersion),
+					DisableAutomaticCompactions: true,
 				}
 
 				var err error
diff --git a/internal/metamorphic/meta_test.go b/internal/metamorphic/meta_test.go
index 7eb694ad..3a9a6b12 100644
--- a/internal/metamorphic/meta_test.go
+++ b/internal/metamorphic/meta_test.go
@@ -148,10 +148,6 @@ func testMetaRun(t *testing.T, runDir string, seed uint64, historyPath string) {
 	// Use an archive cleaner to ease post-mortem debugging.
 	opts.Cleaner = base.ArchiveCleaner{}
 
-	// Enable experimental range keys support.
-	rangeKeysArena := &pebble.RangeKeysArena{}
-	opts.Experimental.RangeKeys = rangeKeysArena
-
 	// Set up the filesystem to use for the test. Note that by default we use an
 	// in-memory FS.
 	if testOpts.useDisk {
diff --git a/iterator_test.go b/iterator_test.go
index b65701bc..322a0f9a 100644
--- a/iterator_test.go
+++ b/iterator_test.go
@@ -1736,7 +1736,6 @@ func newTestkeysDatabase(t *testing.T, ks testkeys.Keyspace) *DB {
 		FS:                 vfs.NewMem(),
 		FormatMajorVersion: FormatRangeKeys,
 	}
-	dbOpts.Experimental.RangeKeys = new(RangeKeysArena)
 	d, err := Open("", dbOpts)
 	require.NoError(t, err)
 
@@ -2056,7 +2055,6 @@ func BenchmarkIteratorScan(b *testing.B) {
 					FS:                 vfs.NewMem(),
 					FormatMajorVersion: FormatNewest,
 				}
-				opts.Experimental.RangeKeys = new(RangeKeysArena)
 				opts.DisableAutomaticCompactions = true
 				d, err := Open("", opts)
 				require.NoError(b, err)
@@ -2112,7 +2110,6 @@ func BenchmarkIteratorSeekNoRangeKeys(b *testing.B) {
 		FS:                 vfs.NewMem(),
 		FormatMajorVersion: FormatNewest,
 	}
-	opts.Experimental.RangeKeys = new(RangeKeysArena)
 	d, err := Open("", opts)
 	require.NoError(b, err)
 	defer func() { require.NoError(b, d.Close()) }()
diff --git a/range_keys_test.go b/range_keys_test.go
index 2d4b89cc..3d472356 100644
--- a/range_keys_test.go
+++ b/range_keys_test.go
@@ -41,7 +41,6 @@ func TestRangeKeys(t *testing.T) {
 				Comparer:           testkeys.Comparer,
 				FormatMajorVersion: FormatRangeKeys,
 			}
-			opts.Experimental.RangeKeys = new(RangeKeysArena)
 
 			for _, cmdArgs := range td.CmdArgs {
 				if cmdArgs.Key != "format-major-version" {
diff --git a/table_stats_test.go b/table_stats_test.go
index ed64d06d..cf28043b 100644
--- a/table_stats_test.go
+++ b/table_stats_test.go
@@ -30,7 +30,6 @@ func TestTableStats(t *testing.T) {
 	}
 	opts.DisableAutomaticCompactions = true
 	opts.Comparer = testkeys.Comparer
-	opts.Experimental.RangeKeys = new(RangeKeysArena)
 	opts.FormatMajorVersion = FormatRangeKeys
 
 	d, err := Open("", opts)