From 3fded19e9945a9334cdd5c5cea758477d15ebba5 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 29 Jul 2024 19:12:37 -0700 Subject: [PATCH] sstable: improve TrivialReaderProvider We improve the `TrivialReaderProvider` implementation to avoid allocation and unexport the type. --- external_iterator.go | 2 +- level_iter_test.go | 2 +- merging_iter_test.go | 2 +- sstable/block_property_test.go | 4 ++-- sstable/random_test.go | 2 +- sstable/reader.go | 2 +- sstable/reader_iter_test.go | 4 ++-- sstable/reader_test.go | 12 ++++++------ sstable/value_block.go | 23 +++++++++++++++-------- 9 files changed, 30 insertions(+), 23 deletions(-) diff --git a/external_iterator.go b/external_iterator.go index e667e9100b..7d60adaf8c 100644 --- a/external_iterator.go +++ b/external_iterator.go @@ -160,7 +160,7 @@ func createExternalPointIter(ctx context.Context, it *Iterator) (topLevelIterato ctx, transforms, it.opts.LowerBound, it.opts.UpperBound, nil, /* BlockPropertiesFilterer */ sstable.NeverUseFilterBlock, &it.stats.InternalStats, it.opts.CategoryAndQoS, nil, - sstable.TrivialReaderProvider{Reader: r}) + sstable.MakeTrivialReaderProvider(r)) if err != nil { return nil, err } diff --git a/level_iter_test.go b/level_iter_test.go index 27436e5c2f..f23809ddf9 100644 --- a/level_iter_test.go +++ b/level_iter_test.go @@ -176,7 +176,7 @@ func (lt *levelIterTest) newIters( iter, err := lt.readers[file.FileNum].NewIterWithBlockPropertyFiltersAndContextEtc( ctx, transforms, opts.LowerBound, opts.UpperBound, nil, sstable.AlwaysUseFilterBlock, iio.stats, sstable.CategoryAndQoS{}, - nil, sstable.TrivialReaderProvider{Reader: lt.readers[file.FileNum]}) + nil, sstable.MakeTrivialReaderProvider(lt.readers[file.FileNum])) if err != nil { return iterSet{}, errors.CombineErrors(err, set.CloseAll()) } diff --git a/merging_iter_test.go b/merging_iter_test.go index 9cabc5e857..e2ffdeb73d 100644 --- a/merging_iter_test.go +++ b/merging_iter_test.go @@ -178,7 +178,7 @@ func TestMergingIterCornerCases(t *testing.T) { set.point, err = r.NewIterWithBlockPropertyFilters( sstable.NoTransforms, opts.GetLowerBound(), opts.GetUpperBound(), nil, sstable.AlwaysUseFilterBlock, iio.stats, - sstable.CategoryAndQoS{}, nil, sstable.TrivialReaderProvider{Reader: r}) + sstable.CategoryAndQoS{}, nil, sstable.MakeTrivialReaderProvider(r)) if err != nil { return iterSet{}, errors.CombineErrors(err, set.CloseAll()) } diff --git a/sstable/block_property_test.go b/sstable/block_property_test.go index 9c9592334b..f428fa76f3 100644 --- a/sstable/block_property_test.go +++ b/sstable/block_property_test.go @@ -973,7 +973,7 @@ func TestBlockProperties(t *testing.T) { } iter, err := r.NewIterWithBlockPropertyFilters( NoTransforms, lower, upper, filterer, NeverUseFilterBlock, &stats, - CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}) + CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r)) if err != nil { return err.Error() } @@ -1055,7 +1055,7 @@ func TestBlockProperties_BoundLimited(t *testing.T) { } iter, err := r.NewIterWithBlockPropertyFilters( NoTransforms, lower, upper, filterer, NeverUseFilterBlock, &stats, - CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}) + CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r)) if err != nil { return err.Error() } diff --git a/sstable/random_test.go b/sstable/random_test.go index 387107dc40..a13dc1c90b 100644 --- a/sstable/random_test.go +++ b/sstable/random_test.go @@ -108,7 +108,7 @@ func runErrorInjectionTest(t *testing.T, seed int64) { &stats, CategoryAndQoS{}, nil, /* CategoryStatsCollector */ - TrivialReaderProvider{r}, + MakeTrivialReaderProvider(r), ) require.NoError(t, err) defer it.Close() diff --git a/sstable/reader.go b/sstable/reader.go index 8ccd4e8842..d1de22b455 100644 --- a/sstable/reader.go +++ b/sstable/reader.go @@ -250,7 +250,7 @@ func (r *Reader) NewIter(transforms IterTransforms, lower, upper []byte) (Iterat // likely isn't a cache set up. return r.NewIterWithBlockPropertyFilters( transforms, lower, upper, nil, AlwaysUseFilterBlock, - nil /* stats */, CategoryAndQoS{}, nil /* statsCollector */, TrivialReaderProvider{Reader: r}) + nil /* stats */, CategoryAndQoS{}, nil /* statsCollector */, MakeTrivialReaderProvider(r)) } // NewCompactionIter returns an iterator similar to NewIter but it also increments diff --git a/sstable/reader_iter_test.go b/sstable/reader_iter_test.go index f6fec5013a..598c6d318e 100644 --- a/sstable/reader_iter_test.go +++ b/sstable/reader_iter_test.go @@ -63,7 +63,7 @@ func TestIteratorErrorOnInit(t *testing.T) { &stats, CategoryAndQoS{}, nil, /* statsCollector */ - TrivialReaderProvider{Reader: r}, + MakeTrivialReaderProvider(r), &pool, ) require.Error(t, err) @@ -78,7 +78,7 @@ func TestIteratorErrorOnInit(t *testing.T) { &stats, CategoryAndQoS{}, nil, /* statsCollector */ - TrivialReaderProvider{Reader: r}, + MakeTrivialReaderProvider(r), &pool, ) require.Error(t, err) diff --git a/sstable/reader_test.go b/sstable/reader_test.go index 33702e61ed..2f28c01423 100644 --- a/sstable/reader_test.go +++ b/sstable/reader_test.go @@ -450,7 +450,7 @@ func runVirtualReaderTest(t *testing.T, path string, blockSize, indexBlockSize i transforms := IterTransforms{SyntheticSuffix: syntheticSuffix} iter, err := v.NewIterWithBlockPropertyFiltersAndContextEtc( context.Background(), transforms, lower, upper, filterer, NeverUseFilterBlock, - &stats, CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}) + &stats, CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r)) if err != nil { return err.Error() } @@ -837,7 +837,7 @@ func runTestReader(t *testing.T, o WriterOptions, dir string, r *Reader, printVa &stats, CategoryAndQoS{}, nil, - TrivialReaderProvider{Reader: r}, + MakeTrivialReaderProvider(r), ) if err != nil { return err.Error() @@ -977,7 +977,7 @@ func TestCompactionIteratorSetupForCompaction(t *testing.T) { var pool block.BufferPool pool.Init(5) citer, err := r.NewCompactionIter( - NoTransforms, CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}, &pool) + NoTransforms, CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r), &pool) require.NoError(t, err) switch i := citer.(type) { case *compactionIterator: @@ -1033,7 +1033,7 @@ func TestReadaheadSetupForV3TablesWithMultipleVersions(t *testing.T) { var pool block.BufferPool pool.Init(5) citer, err := r.NewCompactionIter( - NoTransforms, CategoryAndQoS{}, nil, TrivialReaderProvider{Reader: r}, &pool) + NoTransforms, CategoryAndQoS{}, nil, MakeTrivialReaderProvider(r), &pool) require.NoError(t, err) defer citer.Close() i := citer.(*compactionIterator) @@ -1328,7 +1328,7 @@ func TestRandomizedPrefixSuffixRewriter(t *testing.T) { block.IterTransforms{SyntheticSuffix: syntheticSuffix, SyntheticPrefix: syntheticPrefix}, nil, nil, nil, AlwaysUseFilterBlock, nil, CategoryAndQoS{}, nil, - TrivialReaderProvider{Reader: eReader}, &virtualState{ + MakeTrivialReaderProvider(eReader), &virtualState{ lower: base.MakeInternalKey([]byte("_"), base.SeqNumMax, base.InternalKeyKindSet), upper: base.MakeRangeDeleteSentinelKey([]byte("~~~~~~~~~~~~~~~~")), }) @@ -2451,7 +2451,7 @@ func BenchmarkIteratorScanObsolete(b *testing.B) { iter, err := r.NewIterWithBlockPropertyFiltersAndContextEtc( context.Background(), transforms, nil, nil, filterer, AlwaysUseFilterBlock, nil, CategoryAndQoS{}, nil, - TrivialReaderProvider{Reader: r}) + MakeTrivialReaderProvider(r)) require.NoError(b, err) b.ResetTimer() for i := 0; i < b.N; i++ { diff --git a/sstable/value_block.go b/sstable/value_block.go index f748bc229b..308a7ce5a6 100644 --- a/sstable/value_block.go +++ b/sstable/value_block.go @@ -719,21 +719,28 @@ type ReaderProvider interface { Close() } -// TrivialReaderProvider implements ReaderProvider for a Reader that will -// outlive the top-level iterator in the iterator tree. -type TrivialReaderProvider struct { - *Reader +// MakeTrivialReaderProvider creates a ReaderProvider which always returns the +// given reader. It should be used when the Reader will outlive the iterator +// tree. +func MakeTrivialReaderProvider(r *Reader) ReaderProvider { + return (*trivialReaderProvider)(r) } -var _ ReaderProvider = TrivialReaderProvider{} +// trivialReaderProvider implements ReaderProvider for a Reader that will +// outlive the top-level iterator in the iterator tree. +// +// Defining the type in this manner (as opposed to a struct) avoids allocation. +type trivialReaderProvider Reader + +var _ ReaderProvider = (*trivialReaderProvider)(nil) // GetReader implements ReaderProvider. -func (trp TrivialReaderProvider) GetReader(ctx context.Context) (*Reader, error) { - return trp.Reader, nil +func (trp *trivialReaderProvider) GetReader(ctx context.Context) (*Reader, error) { + return (*Reader)(trp), nil } // Close implements ReaderProvider. -func (trp TrivialReaderProvider) Close() {} +func (trp *trivialReaderProvider) Close() {} // valueBlockReader is used to retrieve values in value // blocks. It is used when the sstable was written with