add BenchmarkReceive10kRowsCompress #1704
Conversation
WalkthroughThe changes introduce a new benchmark step in the GitHub Actions workflow to run Go benchmarks as part of the CI process. In the Go benchmark test file, several functions are renamed for clarity and consistency, and benchmarking practices are updated to use Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Go Test Runner
GitHub Actions->>Go Test Runner: Run tests (go test)
GitHub Actions->>Go Test Runner: Run benchmarks (go test -run '^$' -bench .)
GitHub Actions->>Go Test Runner: Upload coverage
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
benchmark_test.go (2)
74-74: Consider using consistent parameter naming.The parameter name
comprhere differs from thecompressused ininitDB. For consistency across the codebase, consider using the same parameter name in both functions.-func benchmarkQuery(b *testing.B, compr bool) { +func benchmarkQuery(b *testing.B, compress bool) {
388-448: Great refactoring of the row benchmark with several optimizations.The refactoring of
BenchmarkReceiveMassiveRowsinto a helperbenchmark10kRowsfunction with several optimizations:
- Pre-filling the repeated string values in the args slice once (lines 402-405)
- Moving variable declarations outside the loop to reduce allocations (lines 433-434)
- Proper resource cleanup with
rows.Close()in all cases- Using
b.Run()to avoid repeating heavy setupThere's a minor typo in the comment on line 421: "fot this purpose" should be "for this purpose".
- // Go 1.24 introduced b.Loop() fot this purpose. But we keep this + // Go 1.24 introduced b.Loop() for this purpose. But we keep this
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)benchmark_test.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test (ubuntu-latest, 1.23, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, 8.4)
- GitHub Check: test (macos-latest, 1.24, 8.4)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.6)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.4)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.11)
- GitHub Check: test (macos-latest, 1.24, 9.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.4)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.1)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.5)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, 5.7)
🔇 Additional comments (7)
.github/workflows/test.yml (1)
99-102: Good addition of benchmark step to CI pipeline.Adding a dedicated benchmark step ensures benchmark performance is tracked as part of continuous integration. The placement after tests but before coverage upload is appropriate, and using
-run '^$'to skip regular tests is the correct approach.benchmark_test.go (6)
49-49: LGTM! Parameter name simplification.The parameter rename from
useCompressiontocompressmakes the code more concise while maintaining clarity.
67-72: LGTM! Better function naming.Renaming from
BenchmarkQueryCompressiontoBenchmarkQueryCompressedand updating the helper function name tobenchmarkQueryimproves clarity and follows the pattern used by other benchmarks.
129-130: LGTM! Improved benchmark timing practices.Using
b.ReportAllocs()andb.ResetTimer()after setup instead of stopping the timer before setup follows better Go benchmarking practices. This approach consistently measures the actual operation while excluding setup time.
166-168: LGTM! Consistent benchmark timing practices.The consistent use of
b.ReportAllocs()andb.ResetTimer()across all benchmarks improves the accuracy and reliability of benchmark results.
202-203: LGTM! Consistent benchmark timing practices.Maintaining consistent timing practices across all benchmarks.
450-457: LGTM! Clear naming and addition of compressed benchmark variant.The new benchmark names
BenchmarkReceive10kRowsandBenchmarkReceive10kRowsCompressedclearly indicate what's being tested. Adding a compressed variant provides valuable performance comparison insights.
methane
force-pushed
the
fix-bench-massiverows
branch
from
89859a2 to
3cb6049
Compare
* Rename BenchmarkReceiveMassiveRows to BenchmarkReceive10kRows * Add BenchmarkReceive10kRowsCompress that run BenchmarkReceiveMassiveRows with compression * Other tiny benchmark improvements.
* Rename BenchmarkReceiveMassiveRows to BenchmarkReceive10kRows * Add BenchmarkReceive10kRowsCompress that run BenchmarkReceiveMassiveRows with compression * Other tiny benchmark improvements.
Description
Checklist
Summary by CodeRabbit