logstore: pool MVCCStats to avoid hot path allocs#113742
logstore: pool MVCCStats to avoid hot path allocs#113742craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
33530bc to
a176594
Compare
Before this commit, logstore.logAppend allocated MVCCStats on heap despite its lifetime being confined within this function's scope. This commit switches to allocating MVCCStats on a sync.Pool. Since logAppend already has another pooled roachpb.Value with exactly the same lifetime, coalesce the allocation of both and put them in the same pool. This makes the cost of this change zero. Microbenchmark results: ``` // before BenchmarkLogStore_StoreEntries/bytes=1.0_KiB-24 837254 1270 ns/op 333 B/op 1 allocs/op // after BenchmarkLogStore_StoreEntries/bytes=1.0_KiB-24 1000000 1153 ns/op 157 B/op 0 allocs/op ``` Epic: none Release note: none
a176594 to
bf28d2e
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Nice that we could reuse the pool.
| return new(struct { | ||
| roachpb.Value | ||
| enginepb.MVCCStats | ||
| }) |
There was a problem hiding this comment.
nit: may as well give this type a name, but don't really care either way.
There was a problem hiding this comment.
Meh, I think that's one of the cases where it's somewhat beneficial to not introduce a type explicitly (so that there is zero encouragement to use it elsewhere). There is exactly one caller/user of this type, and it's tied to the pool, and the pool is tied to a single function (also a reason why I renamed the pool after the function's name).
|
Btw, see |
|
bors r=erikgrinaker |
|
Build succeeded: |
Before this commit,
logstore.logAppendallocatedMVCCStatson heap despite its lifetime being confined within this function's scope.This commit switches to allocating
MVCCStatson async.Pool. SincelogAppendalready has another pooledroachpb.Valuewith exactly the same lifetime, coalesce the allocation of both and put them in the same pool. This makes the cost of this change zero.Microbenchmark results:
Part of #111561
Epic: none
Release note: none