From 0111cb618dd83d408e3af4fb0f7904d309ca517e Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 19 Apr 2021 09:43:33 -0400 Subject: [PATCH] storage: pool pebbleReadOnly allocations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces object pooling for `pebbleReadOnly` allocation avoidance. I found this to be important both because it avoids the initial `pebbleReadOnly` allocation, but also because it allows the memory recycling inside of each `pebbleIterator` owned by a `pebbleReadOnly` to work correctly. I found this while running a few microbenchmarks and noticing that the lockTable's calls to `intentInterleavingReader` were resulting in a large number of heap allocations in `(*pebbleReadOnly).NewEngineIterator`. This was because `lowerBoundBuf` and `upperBoundBuf` were always nil and so each append (all 4 of them) in `(*pebbleIterator).init` was causing an allocation. ``` name old time/op new time/op delta KV/Scan/Native/rows=1-16 30.9µs ± 4% 29.9µs ± 6% -3.29% (p=0.000 n=20+20) KV/Scan/Native/rows=100-16 54.2µs ± 4% 52.7µs ± 5% -2.84% (p=0.002 n=20+20) KV/Scan/Native/rows=10-16 34.0µs ± 3% 33.1µs ± 6% -2.64% (p=0.001 n=20+20) KV/Scan/Native/rows=1000-16 253µs ± 5% 255µs ± 5% ~ (p=0.659 n=20+20) KV/Scan/Native/rows=10000-16 2.16ms ± 4% 2.14ms ± 3% ~ (p=0.072 n=20+20) name old alloc/op new alloc/op delta KV/Scan/Native/rows=1-16 8.69kB ± 0% 7.49kB ± 0% -13.79% (p=0.000 n=20+19) KV/Scan/Native/rows=10-16 10.1kB ± 0% 8.9kB ± 0% -11.87% (p=0.000 n=20+18) KV/Scan/Native/rows=100-16 22.7kB ± 0% 21.5kB ± 0% -5.29% (p=0.000 n=17+19) KV/Scan/Native/rows=1000-16 174kB ± 0% 172kB ± 0% -0.66% (p=0.000 n=19+19) KV/Scan/Native/rows=10000-16 1.51MB ± 0% 1.51MB ± 0% -0.05% (p=0.000 n=16+19) name old allocs/op new allocs/op delta KV/Scan/Native/rows=1-16 71.0 ± 0% 62.0 ± 0% -12.68% (p=0.000 n=20+20) KV/Scan/Native/rows=10-16 75.0 ± 0% 66.0 ± 0% -12.00% (p=0.000 n=20+19) KV/Scan/Native/rows=100-16 79.0 ± 0% 70.0 ± 0% -11.39% (p=0.000 n=19+19) KV/Scan/Native/rows=1000-16 87.8 ± 1% 79.0 ± 0% -9.97% (p=0.000 n=20+16) KV/Scan/Native/rows=10000-16 113 ± 2% 103 ± 2% -8.19% (p=0.000 n=17+19) ``` We may want to consider this as a candidate to backport to release-21.1, because the lack of pooling here was even more detrimental with the separated lockTable, which creates a separate EngineIterator. So this may have a small impact on #62078. Release note (performance improvement): A series of heap allocations performed when serving read-only queries are now avoided. --- pkg/storage/pebble.go | 47 ++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index 200f7be7bc8c..4a500d9d19e0 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -20,6 +20,7 @@ import ( "sort" "strconv" "strings" + "sync" "sync/atomic" "time" @@ -1074,18 +1075,7 @@ func (p *Pebble) NewBatch() Batch { // NewReadOnly implements the Engine interface. func (p *Pebble) NewReadOnly() ReadWriter { - // TODO(sumeer): a sync.Pool for pebbleReadOnly would save on allocations - // for the underlying pebbleIterators. - return &pebbleReadOnly{ - parent: p, - // Defensively set reusable=true. One has to be careful about this since - // an accidental false value would cause these iterators, that are value - // members of pebbleReadOnly, to be put in the pebbleIterPool. - prefixIter: pebbleIterator{reusable: true}, - normalIter: pebbleIterator{reusable: true}, - prefixEngineIter: pebbleIterator{reusable: true}, - normalEngineIter: pebbleIterator{reusable: true}, - } + return newPebbleReadOnly(p) } // NewUnindexedBatch implements the Engine interface. @@ -1273,6 +1263,37 @@ type pebbleReadOnly struct { var _ ReadWriter = &pebbleReadOnly{} +var pebbleReadOnlyPool = sync.Pool{ + New: func() interface{} { + return &pebbleReadOnly{ + // Defensively set reusable=true. One has to be careful about this since + // an accidental false value would cause these iterators, that are value + // members of pebbleReadOnly, to be put in the pebbleIterPool. + prefixIter: pebbleIterator{reusable: true}, + normalIter: pebbleIterator{reusable: true}, + prefixEngineIter: pebbleIterator{reusable: true}, + normalEngineIter: pebbleIterator{reusable: true}, + } + }, +} + +// Instantiates a new pebbleReadOnly. +func newPebbleReadOnly(parent *Pebble) *pebbleReadOnly { + p := pebbleReadOnlyPool.Get().(*pebbleReadOnly) + // When p is a reused pebbleReadOnly from the pool, the iter fields preserve + // the original reusable=true that was set above in pebbleReadOnlyPool.New(), + // and some buffers that are safe to reuse. Everything else has been reset by + // pebbleIterator.destroy(). + *p = pebbleReadOnly{ + parent: parent, + prefixIter: p.prefixIter, + normalIter: p.normalIter, + prefixEngineIter: p.prefixEngineIter, + normalEngineIter: p.normalEngineIter, + } + return p +} + func (p *pebbleReadOnly) Close() { if p.closed { panic("closing an already-closed pebbleReadOnly") @@ -1285,6 +1306,8 @@ func (p *pebbleReadOnly) Close() { p.normalIter.destroy() p.prefixEngineIter.destroy() p.normalEngineIter.destroy() + + pebbleReadOnlyPool.Put(p) } func (p *pebbleReadOnly) Closed() bool {