Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

db: avoid keyspanimpl.TableNewSpanIter closure allocation #3584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions compaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ func (c *compaction) allowZeroSeqNum() bool {

// newInputIters returns an iterator over all the input tables in a compaction.
func (c *compaction) newInputIters(
newIters tableNewIters, newRangeKeyIter keyspanimpl.TableNewSpanIter,
newIters tableNewIters,
) (
pointIter internalIterator,
rangeDelIter, rangeKeyIter keyspan.FragmentIterator,
Expand Down Expand Up @@ -778,6 +778,7 @@ func (c *compaction) newInputIters(
}
}
}()
ctx := context.Background()
iterOpts := IterOptions{
CategoryAndQoS: sstable.CategoryAndQoS{
Category: "pebble-compaction",
Expand Down Expand Up @@ -809,7 +810,7 @@ func (c *compaction) newInputIters(
// initRangeDel, the levelIter will close and forget the range
// deletion iterator when it steps on to a new file. Surfacing range
// deletions to compactions are handled below.
iters = append(iters, newLevelIter(context.Background(),
iters = append(iters, newLevelIter(ctx,
iterOpts, c.comparer, newIters, level.files.Iter(), l, internalIterOpts{
compaction: true,
bufferPool: &c.bufferPool,
Expand Down Expand Up @@ -875,8 +876,8 @@ func (c *compaction) newInputIters(
}
if hasRangeKeys {
li := &keyspanimpl.LevelIter{}
newRangeKeyIterWrapper := func(file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
iter, err := newRangeKeyIter(file, iterOptions)
newRangeKeyIterWrapper := func(ctx context.Context, file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
iter, err := newIters.NewRangeKeyIter(ctx, file, iterOptions)
if err != nil {
return nil, err
} else if iter == nil {
Expand All @@ -896,7 +897,7 @@ func (c *compaction) newInputIters(
// in this sstable must wholly lie within the file's bounds.
return iter, err
}
li.Init(keyspan.SpanIterOptions{}, c.cmp, newRangeKeyIterWrapper, level.files.Iter(), l, manifest.KeyTypeRange)
li.Init(ctx, keyspan.SpanIterOptions{}, c.cmp, newRangeKeyIterWrapper, level.files.Iter(), l, manifest.KeyTypeRange)
rangeKeyIters = append(rangeKeyIters, li)
}
return nil
Expand Down Expand Up @@ -2556,7 +2557,7 @@ func (d *DB) runCompaction(
c.bufferPool.Init(12)
defer c.bufferPool.Release()

pointIter, rangeDelIter, rangeKeyIter, err := c.newInputIters(d.newIters, d.tableNewRangeKeyIter)
pointIter, rangeDelIter, rangeKeyIter, err := c.newInputIters(d.newIters)
if err != nil {
return nil, pendingOutputs, stats, err
}
Expand Down
2 changes: 1 addition & 1 deletion compaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2460,7 +2460,7 @@ func TestCompactionCheckOrdering(t *testing.T) {
return iterSet{point: &errorIter{}}, nil
}
result := "OK"
_, _, _, err := c.newInputIters(newIters, nil)
_, _, _, err := c.newInputIters(newIters)
if err != nil {
result = fmt.Sprint(err)
}
Expand Down
32 changes: 13 additions & 19 deletions db.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"github.com/cockroachdb/pebble/internal/invalidating"
"github.com/cockroachdb/pebble/internal/invariants"
"github.com/cockroachdb/pebble/internal/keyspan"
"github.com/cockroachdb/pebble/internal/keyspan/keyspanimpl"
"github.com/cockroachdb/pebble/internal/manifest"
"github.com/cockroachdb/pebble/internal/manual"
"github.com/cockroachdb/pebble/objstorage"
Expand Down Expand Up @@ -297,9 +296,8 @@ type DB struct {
fileLock *Lock
dataDir vfs.File

tableCache *tableCacheContainer
newIters tableNewIters
tableNewRangeKeyIter keyspanimpl.TableNewSpanIter
tableCache *tableCacheContainer
newIters tableNewIters

commit *commitPipeline

Expand Down Expand Up @@ -1043,7 +1041,6 @@ func (d *DB) newIter(
}
var readState *readState
var newIters tableNewIters
var newIterRangeKey keyspanimpl.TableNewSpanIter
if !internalOpts.batch.batchOnly {
// Grab and reference the current readState. This prevents the underlying
// files in the associated version from being deleted if there is a current
Expand All @@ -1067,7 +1064,6 @@ func (d *DB) newIter(
seqNum = d.mu.versions.visibleSeqNum.Load()
}
newIters = d.newIters
newIterRangeKey = d.tableNewRangeKeyIter
}

// Bundle various structures under a single umbrella in order to allocate
Expand All @@ -1086,7 +1082,6 @@ func (d *DB) newIter(
boundsBuf: buf.boundsBuf,
batch: batch,
newIters: newIters,
newIterRangeKey: newIterRangeKey,
seqNum: seqNum,
batchOnlyIter: internalOpts.batch.batchOnly,
}
Expand Down Expand Up @@ -1184,7 +1179,7 @@ func finishInitializingIter(ctx context.Context, buf *iterAlloc) *Iterator {
if dbi.rangeKey == nil {
dbi.rangeKey = iterRangeKeyStateAllocPool.Get().(*iteratorRangeKeyState)
dbi.rangeKey.init(dbi.comparer.Compare, dbi.comparer.Split, &dbi.opts)
dbi.constructRangeKeyIter()
dbi.constructRangeKeyIter(ctx)
} else {
dbi.rangeKey.iterConfig.SetBounds(dbi.opts.LowerBound, dbi.opts.UpperBound)
}
Expand Down Expand Up @@ -1309,17 +1304,16 @@ func (d *DB) newInternalIter(
// them together.
buf := iterAllocPool.Get().(*iterAlloc)
dbi := &scanInternalIterator{
ctx: ctx,
db: d,
comparer: d.opts.Comparer,
merge: d.opts.Merger.Merge,
readState: readState,
version: sOpts.vers,
alloc: buf,
newIters: d.newIters,
newIterRangeKey: d.tableNewRangeKeyIter,
seqNum: seqNum,
mergingIter: &buf.merging,
ctx: ctx,
db: d,
comparer: d.opts.Comparer,
merge: d.opts.Merger.Merge,
readState: readState,
version: sOpts.vers,
alloc: buf,
newIters: d.newIters,
seqNum: seqNum,
mergingIter: &buf.merging,
}
dbi.opts = *o
dbi.opts.logger = d.opts.Logger
Expand Down
36 changes: 11 additions & 25 deletions flushable.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,9 @@ type flushableList []*flushableEntry
// ingesting sstables which are added to the flushable list.
type ingestedFlushable struct {
// files are non-overlapping and ordered (according to their bounds).
files []physicalMeta
comparer *Comparer
newIters tableNewIters
newRangeKeyIters keyspanimpl.TableNewSpanIter
files []physicalMeta
comparer *Comparer
newIters tableNewIters

// Since the level slice is immutable, we construct and set it once. It
// should be safe to read from slice in future reads.
Expand All @@ -168,11 +167,7 @@ type ingestedFlushable struct {
}

func newIngestedFlushable(
files []*fileMetadata,
comparer *Comparer,
newIters tableNewIters,
newRangeKeyIters keyspanimpl.TableNewSpanIter,
exciseSpan KeyRange,
files []*fileMetadata, comparer *Comparer, newIters tableNewIters, exciseSpan KeyRange,
) *ingestedFlushable {
if invariants.Enabled {
for i := 1; i < len(files); i++ {
Expand All @@ -193,10 +188,9 @@ func newIngestedFlushable(
}

ret := &ingestedFlushable{
files: physicalFiles,
comparer: comparer,
newIters: newIters,
newRangeKeyIters: newRangeKeyIters,
files: physicalFiles,
comparer: comparer,
newIters: newIters,
// slice is immutable and can be set once and used many times.
slice: manifest.NewLevelSliceKeySorted(comparer.Compare, files),
hasRangeKeys: hasRangeKeys,
Expand Down Expand Up @@ -233,16 +227,6 @@ func (s *ingestedFlushable) newFlushIter(*IterOptions) internalIterator {
panic("pebble: not implemented")
}

func (s *ingestedFlushable) constructRangeDelIter(
file *manifest.FileMetadata, _ keyspan.SpanIterOptions,
) (keyspan.FragmentIterator, error) {
iters, err := s.newIters(context.Background(), file, nil, internalIterOpts{}, iterRangeDeletions)
// Note that the keyspan level iter expects a non-nil iterator to be
// returned even if there is an error. So, we return iters.RangeDeletion()
// regardless of the value of err.
return iters.RangeDeletion(), err
}

// newRangeDelIter is part of the flushable interface.
// TODO(bananabrick): Using a level iter instead of a keyspan level iter to
// surface range deletes is more efficient.
Expand All @@ -251,8 +235,9 @@ func (s *ingestedFlushable) constructRangeDelIter(
// the point iterator in constructRangeDeIter is not tracked.
func (s *ingestedFlushable) newRangeDelIter(_ *IterOptions) keyspan.FragmentIterator {
return keyspanimpl.NewLevelIter(
context.Background(),
keyspan.SpanIterOptions{}, s.comparer.Compare,
s.constructRangeDelIter, s.slice.Iter(), manifest.Level(0),
s.newIters.NewRangeDelIter, s.slice.Iter(), manifest.Level(0),
manifest.KeyTypePoint,
)
}
Expand All @@ -264,7 +249,8 @@ func (s *ingestedFlushable) newRangeKeyIter(o *IterOptions) keyspan.FragmentIter
}

return keyspanimpl.NewLevelIter(
keyspan.SpanIterOptions{}, s.comparer.Compare, s.newRangeKeyIters,
context.Background(),
keyspan.SpanIterOptions{}, s.comparer.Compare, s.newIters.NewRangeKeyIter,
s.slice.Iter(), manifest.Level(0), manifest.KeyTypeRange,
)
}
Expand Down
2 changes: 1 addition & 1 deletion flushable_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestIngestedSSTFlushableAPI(t *testing.T) {
}

meta := loadFileMeta(paths)
flushable = newIngestedFlushable(meta, d.opts.Comparer, d.newIters, d.tableNewRangeKeyIter, KeyRange{})
flushable = newIngestedFlushable(meta, d.opts.Comparer, d.newIters, KeyRange{})
return ""
case "iter":
iter := flushable.newIter(nil)
Expand Down
2 changes: 1 addition & 1 deletion ingest.go
Original file line number Diff line number Diff line change
Expand Up @@ -1271,7 +1271,7 @@ func (d *DB) newIngestedFlushableEntry(
}
}

f := newIngestedFlushable(meta, d.opts.Comparer, d.newIters, d.tableNewRangeKeyIter, exciseSpan)
f := newIngestedFlushable(meta, d.opts.Comparer, d.newIters, exciseSpan)

// NB: The logNum/seqNum are the WAL number which we're writing this entry
// to and the sequence number within the WAL which we'll write this entry
Expand Down
15 changes: 12 additions & 3 deletions internal/keyspan/keyspanimpl/level_iter.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package keyspanimpl

import (
"context"
"fmt"

"github.com/cockroachdb/errors"
Expand All @@ -17,13 +18,18 @@ import (
// TableNewSpanIter creates a new iterator for range key spans for the given
// file.
type TableNewSpanIter func(
file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions,
ctx context.Context, file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions,
) (keyspan.FragmentIterator, error)

// LevelIter provides a merged view of spans from sstables in a level.
// It takes advantage of level invariants to only have one sstable span block
// open at one time, opened using the newIter function passed in.
type LevelIter struct {
// The context is stored here since (a) iterators are expected to be
// short-lived (since they pin sstables), (b) plumbing a context into every
// method is very painful, (c) they do not (yet) respect context
// cancellation and are only used for tracing.
ctx context.Context
cmp base.Compare
// Denotes the kind of key the level iterator should read. If the key type
// is KeyTypePoint, the level iterator will read range tombstones (which
Expand Down Expand Up @@ -84,6 +90,7 @@ var _ keyspan.FragmentIterator = (*LevelIter)(nil)

// NewLevelIter returns a LevelIter.
func NewLevelIter(
ctx context.Context,
opts keyspan.SpanIterOptions,
cmp base.Compare,
newIter TableNewSpanIter,
Expand All @@ -92,19 +99,21 @@ func NewLevelIter(
keyType manifest.KeyType,
) *LevelIter {
l := &LevelIter{}
l.Init(opts, cmp, newIter, files, level, keyType)
l.Init(ctx, opts, cmp, newIter, files, level, keyType)
return l
}

// Init initializes a LevelIter.
func (l *LevelIter) Init(
ctx context.Context,
opts keyspan.SpanIterOptions,
cmp base.Compare,
newIter TableNewSpanIter,
files manifest.LevelIterator,
level manifest.Level,
keyType manifest.KeyType,
) {
l.ctx = ctx
l.err = nil
l.level = level
l.tableOpts = opts
Expand Down Expand Up @@ -159,7 +168,7 @@ func (l *LevelIter) loadFile(file *manifest.FileMetadata, dir int) loadFileRetur
return noFileLoaded
}
if indicator != fileAlreadyLoaded {
l.iter, l.err = l.newIter(file, l.tableOpts)
l.iter, l.err = l.newIter(l.ctx, file, l.tableOpts)
if l.wrapFn != nil {
l.iter = l.wrapFn(l.iter)
}
Expand Down
7 changes: 5 additions & 2 deletions internal/keyspan/keyspanimpl/level_iter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package keyspanimpl

import (
"context"
"fmt"
"strings"
"testing"
Expand Down Expand Up @@ -301,7 +302,7 @@ func TestLevelIterEquivalence(t *testing.T) {
metas = append(metas, meta)
}

tableNewIters := func(file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
tableNewIters := func(_ context.Context, file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
return keyspan.NewIter(base.DefaultComparer.Compare, tc.levels[j][file.FileNum-1]), nil
}
// Add all the fileMetadatas to L6.
Expand All @@ -314,6 +315,7 @@ func TestLevelIterEquivalence(t *testing.T) {
v, err := b.Apply(nil, base.DefaultComparer, 0, 0)
require.NoError(t, err)
levelIter.Init(
context.Background(),
keyspan.SpanIterOptions{}, base.DefaultComparer.Compare, tableNewIters,
v.Levels[6].Iter(), 0, manifest.KeyTypeRange,
)
Expand Down Expand Up @@ -440,7 +442,7 @@ func TestLevelIter(t *testing.T) {
}
if iter == nil {
var lastFileNum base.FileNum
tableNewIters := func(file *manifest.FileMetadata, _ keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
tableNewIters := func(_ context.Context, file *manifest.FileMetadata, _ keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
keyType := keyType
spans := level[file.FileNum-1]
if keyType == manifest.KeyTypePoint {
Expand All @@ -458,6 +460,7 @@ func TestLevelIter(t *testing.T) {
v, err := b.Apply(nil, base.DefaultComparer, 0, 0)
require.NoError(t, err)
iter = NewLevelIter(
context.Background(),
keyspan.SpanIterOptions{}, base.DefaultComparer.Compare,
tableNewIters, v.Levels[6].Iter(), 6, keyType,
)
Expand Down
3 changes: 0 additions & 3 deletions iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/cockroachdb/pebble/internal/humanize"
"github.com/cockroachdb/pebble/internal/invariants"
"github.com/cockroachdb/pebble/internal/keyspan"
"github.com/cockroachdb/pebble/internal/keyspan/keyspanimpl"
"github.com/cockroachdb/pebble/internal/manifest"
"github.com/cockroachdb/pebble/internal/rangekeystack"
"github.com/cockroachdb/pebble/sstable"
Expand Down Expand Up @@ -233,7 +232,6 @@ type Iterator struct {
// Non-nil if this Iterator includes a Batch.
batch *Batch
newIters tableNewIters
newIterRangeKey keyspanimpl.TableNewSpanIter
lazyCombinedIter lazyCombinedIter
seqNum uint64
// batchSeqNum is used by Iterators over indexed batches to detect when the
Expand Down Expand Up @@ -2822,7 +2820,6 @@ func (i *Iterator) CloneWithContext(ctx context.Context, opts CloneOptions) (*It
batch: i.batch,
batchSeqNum: i.batchSeqNum,
newIters: i.newIters,
newIterRangeKey: i.newIterRangeKey,
seqNum: i.seqNum,
}
dbi.processBounds(dbi.opts.LowerBound, dbi.opts.UpperBound)
Expand Down
1 change: 0 additions & 1 deletion open.go
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,6 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
opts.TableCache, d.cacheID, d.objProvider, d.opts, tableCacheSize,
&sstable.CategoryStatsCollector{})
d.newIters = d.tableCache.newIters
d.tableNewRangeKeyIter = tableNewRangeKeyIter(context.TODO(), d.newIters)

var previousOptionsFileNum base.DiskFileNum
var previousOptionsFilename string
Expand Down
Loading
Loading