feat(serverv2): add benchmarks of (old) cacheKV vs branch #22497
Conversation
testinginprod
requested review from
cool-develope,
kocubinski and
a team
as code owners
📝 WalkthroughWalkthroughThe changes introduce enhancements to benchmarking tests in the Changes
Sequence Diagram(s)sequenceDiagram
participant B as Benchmark
participant S as Store
participant C as CacheStack
B->>C: Benchmark_CacheStack_Set()
C->>S: Set(key, value)
S-->>C: Result (stored in sink)
B->>C: Benchmark_Get()
C->>S: Get(key)
S-->>C: Result (stored in sink)
B->>C: Benchmark_GetSparse()
C->>S: Get(unique_key)
S-->>C: Result (stored in sink)
B->>C: Benchmark_Iterate()
C->>S: Iterate()
S-->>C: Key-Value pairs
Warning Tool Failures:Tool Failure Count:Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:Store
C:server/v2
|
@testinginprod your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
server/v2/stf/branch/bench_test.go (3)
35-35: Consider adding more detailed performance metrics.Given the significant performance regression (320x slower) mentioned in the PR objectives, consider adding:
- Memory statistics using
b.ReportAllocs()- Sub-benchmarks for different key patterns
- CPU profiling for detailed analysis
Example enhancement:
func Benchmark_Get(b *testing.B) { var sink any for _, stackSize := range stackSizes { b.Run(fmt.Sprintf("StackSize%d", stackSize), func(b *testing.B) { bs := makeBranchStack(b, stackSize) + // Add CPU profiling + if stackSize == 100 { // Profile only larger sizes + f, _ := os.Create("cpu.prof") + pprof.StartCPUProfile(f) + defer pprof.StopCPUProfile() + } b.ResetTimer() b.ReportAllocs() for i := 0; i < b.N; i++ { sink, _ = bs.Get([]byte{0}) } }) } if sink == nil { b.Fatal("prevent compiler optimization") } }Also applies to: 42-48
51-72: LGTM! Consider adding parallel benchmark variant.The benchmark is well-structured with proper key generation outside the timed section. Given the significant performance improvement (11.77x faster), it would be valuable to also test parallel access patterns.
Add a parallel variant:
+func Benchmark_GetSparse_Parallel(b *testing.B) { + for _, stackSize := range stackSizes { + b.Run(fmt.Sprintf("StackSize%d", stackSize), func(b *testing.B) { + bs := makeBranchStack(b, stackSize) + keys := make([][]byte, b.N) + for i := 0; i < b.N; i++ { + keys[i] = numToBytes(i) + } + b.ResetTimer() + b.ReportAllocs() + b.RunParallel(func(pb *testing.PB) { + var sink any + i := 0 + for pb.Next() { + sink, _ = bs.Get(keys[i%len(keys)]) + i++ + } + if sink == nil { + b.Fatal("prevent compiler optimization") + } + }) + }) + } +}
118-120: LGTM! Consider adding documentation.The numToBytes function is well-implemented using generics and efficient binary encoding.
Add documentation to clarify the encoding format:
+// numToBytes converts an integer to its big-endian binary representation. +// The function always returns an 8-byte slice regardless of the input value. func numToBytes[T ~int](n T) []byte { return binary.BigEndian.AppendUint64(nil, uint64(n)) }store/cachekv/branch_bench_test.go (5)
13-17: Consider using constants and adding documentation.The global variables would benefit from being declared as constants since they're not modified. Additionally, adding documentation would improve clarity about their purpose in the benchmarks.
-var ( +const ( stackSizes = []int{1, 10, 100} elemsInStack = 10 ) + +// stackSizes defines the different stack depths to benchmark +// elemsInStack defines the number of elements to populate in each stack layer
18-29: Consider enhancing benchmark descriptions.While the benchmark implementation is solid, the sub-benchmark descriptions could be more descriptive to better indicate what's being tested.
-b.Run(fmt.Sprintf("StackSize%d", stackSize), func(b *testing.B) { +b.Run(fmt.Sprintf("StackSize=%d/SingleKeySet", stackSize), func(b *testing.B) {
74-96: Add documentation for sink variables.The iterator benchmark correctly uses sink variables to prevent compiler optimizations, but their purpose should be documented.
-var keySink, valueSink any +// keySink and valueSink prevent the compiler from optimizing away the iterator operations +var keySink, valueSink any
98-112: Remove unused parameter in makeBranchStack.The testing.B parameter is not used in the function.
-func makeBranchStack(_ *testing.B, stackSize int) *cachekv.Store { +func makeBranchStack(stackSize int) *cachekv.Store {
114-116: Add documentation for numToBytes function.The generic function should have documentation explaining its purpose and encoding details.
+// numToBytes converts a number to its big-endian byte representation. +// It uses a generic constraint to accept any integer type. func numToBytes[T ~int](n T) []byte {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/stf/branch/bench_test.go(4 hunks)server/v2/stf/branch/store.go(1 hunks)store/cachekv/branch_bench_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/stf/branch/bench_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/stf/branch/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/cachekv/branch_bench_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (6)
server/v2/stf/branch/bench_test.go (2)
107-107: LGTM! Key generation ensures uniqueness across branches.
The key generation logic correctly combines branch and element indices using the efficient numToBytes function.
18-18: LGTM! Compiler optimization prevention is properly implemented.
The sink variable pattern is correctly used to prevent the compiler from optimizing away the benchmark operations.
Let's verify that the benchmark results are stable and not affected by compiler optimizations:
Also applies to: 25-25, 29-31
✅ Verification successful
Sink variable usage follows established patterns across the codebase
The sink variable declaration and usage in server/v2/stf/branch/bench_test.go follows the consistent pattern found across the codebase's benchmark tests. This is a well-established practice in the project, with similar implementations in other benchmark files like store/cachekv/store_bench_test.go and math/dec_test.go.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Run the benchmark multiple times to check result stability
go test -bench=Benchmark_CacheStack_Set -count=5 ./server/v2/stf/branch/... | tee results.txt
# Check if results are consistent (should have similar numbers)
awk '/Benchmark_CacheStack_Set/ { print $3 }' results.txt | sort -n | uniq -c
Length of output: 398
Script:
#!/bin/bash
# Let's verify the sink variable usage pattern in the benchmark tests
rg -U "var\s+sink\s+.*?\n.*?=.*?sink" --type go
# Check for other benchmark functions and their sink usage
ast-grep --pattern 'func Benchmark$_($_) {
$$$
}'
# Look for any other sink variable declarations
rg "var\s+sink\s+" --type go
Length of output: 666
server/v2/stf/branch/store.go (2)
Line range hint 32-36: LGTM: Comment changes improve clarity
The updated comment accurately describes the control flow of the Get method.
Line range hint 32-36: Consider optimizing Get method performance
According to the PR objectives, there's a significant performance regression (320× slower) in _GetCached operations. Consider implementing the following optimizations:
- Add read-through caching to reduce parent store lookups
- Implement bulk operations for batch retrievals
- Add prefetching mechanisms for frequently accessed keys
Let's analyze the usage patterns to identify optimization opportunities:
store/cachekv/branch_bench_test.go (2)
1-11: LGTM! Package structure and imports are well-organized.
The package follows Go conventions with proper import grouping.
31-47: Verify benchmark coverage matches PR objectives.
The Get and GetSparse benchmarks align with the PR objectives showing the 320× slowdown and 11.77× speedup respectively. However, consider adding more granular test cases to investigate the significant performance regression in Get operations.
Consider adding benchmarks with different key sizes and value sizes to better understand the performance characteristics:
+var (
+ benchmarkKeySizes = []int{8, 64, 256} // in bytes
+ benchmarkValSizes = []int{8, 1024, 4096} // in bytes
+)Also applies to: 49-72
| } | ||
| }) | ||
| } | ||
| if sink != nil { |
There was a problem hiding this comment.
You want the opposite, if sink == nil
| }) | ||
| } | ||
| if sink == nil { | ||
| b.Fatal("prevent compiler optimization") |
There was a problem hiding this comment.
Please use instead b.Fatal("Benchmark did not run")
| } | ||
|
|
||
| func Benchmark_GetSparse(b *testing.B) { | ||
| var sink any |
There was a problem hiding this comment.
You need this sink to be a global to ensure writebarriers and also please reset it to nil after usage.
|
|
||
| // Gets the same key from the branch store. | ||
| func Benchmark_Get(b *testing.B) { | ||
| var sink any |
There was a problem hiding this comment.
Needs to global and to be reset after use.
|
|
||
| // Gets always different keys. | ||
| func Benchmark_GetSparse(b *testing.B) { | ||
| var sink any |
There was a problem hiding this comment.
Needs to be global and reset to nil after usage.
Description
Summary of Benchmark Performance Comparison
Bench Test Type:
_CacheStack_Set:This sets the same key in state. Note: Stack size should have no impact here since the top branch is the one where the cache is being set.
StackSize1-14
StackSize10-14
StackSize100-14
Bench Test Type:
_GetCachedGets the same key.
NOTE: CacheKV caches the key on the first get, making getting the same key expensive only the first time but in-expensive other times. This works well only when we fetch the same key. GetSparse shows the opposite view.
StackSize1-14
StackSize10-14
StackSize100-14
Bench Test Type:
_GetSparseFetching always new key from storage.
StackSize1-14
StackSize10-14
StackSize100-14
Bench Test Type:
_IterateStackSize1-14
StackSize10-14
StackSize100-14
Overall Performance
Memory Usage
Bytes per Operation (B/op):
_CacheStack_Set_GetCached_GetSparse_IterateAllocations per Operation (allocs/op):
_CacheStack_Set_GetSparse_IterateConclusion:
_CacheStack_Set,_GetSparse, and_Iteratebenchmarks, especially with larger stack sizes._GetCachedbenchmarks, where the current code is slower than the previous version.This summary provides a detailed comparison of the benchmark tests, highlighting where the current code has improved or declined in performance relative to the previous version.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
cachekvpackage, measuring performance for setting and retrieving values, as well as iteration over key-value pairs.Bug Fixes
Getmethod within theStorestruct to better reflect the logic flow.These enhancements aim to improve performance evaluation and code clarity for users interacting with the caching system.