Skip to content

Commit

Permalink
storage: pool pebbleReadOnly allocations
Browse files Browse the repository at this point in the history
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 cockroachdb#62078.

Release note (performance improvement): A series of heap allocations performed
when serving read-only queries are now avoided.
  • Loading branch information
nvanbenschoten committed Apr 21, 2021
1 parent 0564fea commit 0111cb6
Showing 1 changed file with 35 additions and 12 deletions.
47 changes: 35 additions & 12 deletions pkg/storage/pebble.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"sort"
"strconv"
"strings"
"sync"
"sync/atomic"
"time"

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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")
Expand All @@ -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 {
Expand Down

0 comments on commit 0111cb6

Please sign in to comment.