kv/logstore: avoid heap allocations around non-blocking sync waiter callback #96565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Minor cleanup. This shouldn't have caused issues.
This commit reduces the verbosity of the warning in SyncWaiterLoop.enqueue when the channel send blocks. It also increases the capacity of the channel to reduce the chance that it blocks, even when consumption from the queue is delayed due to slow goroutine scheduling. Blocking here instead of during the call to `batch.CommitNoSyncWait` is not a serious problem, except that it makes the behavior of `LogStore.StoreEntries` more complex and difficult to reason about.
|
This change is |
tbg
approved these changes
Feb 6, 2023
…allback This commit structures the non-blocking sync callback provided to the Raft log `SyncWaiterLoop` as a struct with a method that satisfies an interface (i.e. a functor) instead of an anonymous function. This provides more control over the memory layout of the callback and prevents individual fields escaping to the heap. The change also provides the opportunity to pool the callback to avoid an additional heap allocation. The change has the following effect on microbenchmarks: ``` name old time/op new time/op delta ReplicaProposal/bytes=1.0_KiB,withFollower=false-10 40.1µs ± 3% 39.1µs ± 1% -2.51% (p=0.000 n=10+9) ReplicaProposal/bytes=512_B,withFollower=false-10 38.2µs ± 2% 37.3µs ± 4% -2.48% (p=0.015 n=10+10) ReplicaProposal/bytes=256_B,withFollower=false-10 37.1µs ± 1% 36.2µs ± 3% -2.32% (p=0.000 n=10+10) ReplicaProposal/bytes=256_B,withFollower=true-10 52.2µs ± 1% 51.2µs ± 1% -1.91% (p=0.000 n=10+10) ReplicaProposal/bytes=1.0_KiB,withFollower=true-10 58.5µs ± 2% 57.5µs ± 2% -1.79% (p=0.001 n=10+10) ReplicaProposal/bytes=512_B,withFollower=true-10 53.8µs ± 2% 52.8µs ± 1% -1.74% (p=0.000 n=10+10) name old speed new speed delta ReplicaProposal/bytes=512_B,withFollower=false-10 13.4MB/s ± 2% 13.7MB/s ± 4% +2.57% (p=0.016 n=10+10) ReplicaProposal/bytes=1.0_KiB,withFollower=false-10 25.5MB/s ± 2% 26.2MB/s ± 1% +2.57% (p=0.000 n=10+9) ReplicaProposal/bytes=256_B,withFollower=false-10 6.91MB/s ± 1% 7.07MB/s ± 3% +2.36% (p=0.000 n=10+10) ReplicaProposal/bytes=256_B,withFollower=true-10 4.90MB/s ± 1% 5.00MB/s ± 1% +1.96% (p=0.000 n=10+10) ReplicaProposal/bytes=1.0_KiB,withFollower=true-10 17.5MB/s ± 2% 17.8MB/s ± 1% +1.82% (p=0.001 n=10+10) ReplicaProposal/bytes=512_B,withFollower=true-10 9.52MB/s ± 2% 9.69MB/s ± 1% +1.76% (p=0.000 n=10+10) name old alloc/op new alloc/op delta ReplicaProposal/bytes=256_B,withFollower=false-10 14.6kB ± 0% 12.8kB ± 0% -12.73% (p=0.000 n=10+10) ReplicaProposal/bytes=256_B,withFollower=true-10 35.0kB ± 1% 30.6kB ± 1% -12.59% (p=0.000 n=10+10) ReplicaProposal/bytes=512_B,withFollower=true-10 42.9kB ± 0% 38.6kB ± 1% -10.04% (p=0.000 n=8+10) ReplicaProposal/bytes=512_B,withFollower=false-10 18.5kB ± 2% 16.8kB ± 1% -9.19% (p=0.000 n=10+10) ReplicaProposal/bytes=1.0_KiB,withFollower=true-10 60.6kB ± 1% 55.9kB ± 1% -7.76% (p=0.000 n=10+10) ReplicaProposal/bytes=1.0_KiB,withFollower=false-10 27.5kB ± 2% 25.6kB ± 2% -7.06% (p=0.000 n=10+10) name old allocs/op new allocs/op delta ReplicaProposal/bytes=512_B,withFollower=false-10 70.0 ± 0% 61.6 ± 1% -12.00% (p=0.000 n=10+10) ReplicaProposal/bytes=256_B,withFollower=false-10 69.0 ± 0% 61.0 ± 0% -11.59% (p=0.000 n=10+10) ReplicaProposal/bytes=1.0_KiB,withFollower=false-10 73.0 ± 0% 65.0 ± 0% -10.96% (p=0.002 n=8+10) ReplicaProposal/bytes=256_B,withFollower=true-10 179 ± 0% 161 ± 0% -10.21% (p=0.000 n=10+7) ReplicaProposal/bytes=512_B,withFollower=true-10 181 ± 1% 162 ± 0% -10.11% (p=0.000 n=9+10) ReplicaProposal/bytes=1.0_KiB,withFollower=true-10 186 ± 0% 168 ± 0% -9.84% (p=0.000 n=9+10) ``` Release note: None Epic: None
nvanbenschoten
force-pushed
the
nvanbenschoten/syncWaiterFix
branch
from
9a3dc02 to
894216b
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit structures the non-blocking sync callback provided to the Raft log
SyncWaiterLoopas a struct with a method that satisfies an interface (i.e. a functor) instead of an anonymous function. This provides more control over the memory layout of the callback and prevents individual fields from escaping to the heap. The change also provides the opportunity to pool the callback to avoid an additional heap allocation.The change has the following effect on microbenchmarks:
Release note: None
Epic: None