storage: reduce allocations when encoding MVCCValue's #120354
Conversation
|
This change is |
stevendanna
force-pushed
the
allocator-plumbing
branch
3 times, most recently
from
51f7bb7 to
5315a0c
Compare
stevendanna
changed the title
stevendanna
force-pushed
the
allocator-plumbing
branch
from
5315a0c to
3ac58b4
Compare
stevendanna
requested review from
itsbilal,
dt,
michae2,
erikgrinaker,
a team and
DrewKimball
and removed request for
a team,
itsbilal,
michae2 and
erikgrinaker
|
Only review the last commit. |
stevendanna
force-pushed
the
allocator-plumbing
branch
4 times, most recently
from
dac6d99 to
277c18f
Compare
pkg/storage/mvcc_value.go
Outdated
| // TODO(ssd): Dedup with EncodeMVCCValue. | ||
| func EncodeMVCCValueToBuf(v MVCCValue, buf []byte) ([]byte, error) { |
There was a problem hiding this comment.
I've opted to duplicate this code for now to avoid touching EncodeMVCCValue at all.
pkg/kv/bulk/sst_batcher.go
Outdated
| @@ -193,6 +194,8 @@ type SSTBatcher struct { | |||
|
|
|||
| asyncAddSSTs ctxgroup.Group | |||
|
|
|||
| valueBuf bufalloc.ByteAllocator | |||
There was a problem hiding this comment.
this is a this is just scratch space, right? this isn't really so much about amortizing lots of small per key allocations into a few rare big ones, but about reusing the same buffer for every key, right?
Just making sure I'm reading it right.
I found bufalloc a little confusing for a pure scratch usecase, but maybe just naming the valScratch would have avoided that?
pkg/storage/mvcc_value.go
Outdated
| // the encoding scheme. | ||
| // | ||
| // If extended encoding is required, the given buffer will be used. If | ||
| // the provided buffer is not large enough an error is |
There was a problem hiding this comment.
2¢:
Instead of an error I'd just allocate a bigger buffer and return that. This makes your TODO easily implemented by EncodeMVCCValueToBuf(x) { return EncodeMVCCValueToBuf(x, nil) }.
And then I'd probably swap the bufalloc for just scratch []byte and call it as scratch, err = EncodeMVCCValueToBuf(x, scratch[:0]) so that if it allocates, you keep the result?
/2¢
There was a problem hiding this comment.
Very nice. This will clean things up a lot.
stevendanna
force-pushed
the
allocator-plumbing
branch
from
277c18f to
6661ab6
Compare
This adds a new function that lets us pass a pre-allocated buffer to use when encoding MVCC values. This is very useful in loops where we might be allocating many MVCC values in a loop, such as in ExportRequest or in SSTBatcher. Before: ``` BenchmarkMVCCExportToSST/importEpochs=false/numKeys=65536/numRevisions=100/exportAllRevisions=true-10 1 1464855709 ns/op 45293904 B/op 39419 allocs/op BenchmarkMVCCExportToSST/importEpochs=true/numKeys=65536/numRevisions=100/exportAllRevisions=true-10 1 1919207875 ns/op 207366256 B/op 6595176 allocs/op ``` After: ``` BenchmarkMVCCExportToSST/importEpochs=false/numKeys=65536/numRevisions=100/exportAllRevisions=true-10 1 1458935583 ns/op 45327256 B/op 39337 allocs/op BenchmarkMVCCExportToSST/importEpochs=true/numKeys=65536/numRevisions=100/exportAllRevisions=true-10 1 1756803250 ns/op 49162648 B/op 41369 allocs/op ``` Epic: none Release note: None
Epic: none Release note: None
stevendanna
force-pushed
the
allocator-plumbing
branch
from
6661ab6 to
e817d29
Compare
|
bors r=dt |
|
Build failed: |
|
bors retry |
|
Build succeeded: |
This adds a new function that lets us pass a buffer to
use when encoding MVCC values. This is very useful in loops where we
might be allocating many MVCC values in a loop, such as in
ExportRequest or in SSTBatcher.
Before:
After:
Epic: none
Release note: None