refactor(tests): Enhance benchmarks by separating allocation checks from main loops and adding input size reporting for throughput metrics#123
Conversation
…itterations and improve allocation checks
WalkthroughThe benchmark functions for Changes
Sequence Diagram(s)sequenceDiagram
participant Benchmark as BenchmarkRunner
participant Function as ToUpper/ToLower
Benchmark->>Benchmark: SetBytes(input size)
Benchmark->>Benchmark: ResetTimer()
loop b.N times
Benchmark->>Function: Call with input string
end
Benchmark->>Benchmark: Check correctness with require.Equal
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
🧪 Generate Unit Tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Summary of Changes
Hello @sixcolors, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the existing Go benchmarks for string case conversion functions (ToUpper and ToLower) to provide more accurate and detailed performance metrics. The changes focus on aligning the benchmark execution with standard Go practices for measuring performance over iterations and improving the precision of memory allocation checks.
Highlights
- Benchmark Iteration Logic: Modified
Benchmark_ToUpperandBenchmark_ToLowerto execute the core string conversion operation forb.Niterations within the main benchmark loop. This change ensures that the primary benchmark run accurately measures the function's execution time over many iterations, aligning with standard Go benchmarking practices. - Improved Allocation Testing: Refactored the allocation checks in
Benchmark_ToUpperandBenchmark_ToLower. Instead of measuring allocations within the main performance loop, a separatetesting.AllocsPerRun(100, func() { ... })call is now used specifically for verifying zero allocations whenupperNoConvorlowerNoConvis true. This isolates allocation measurement from performance measurement, providing clearer and more reliable results. - Enhanced Benchmark Reporting: Added
b.SetBytes(int64(len(tc.input)))to allToUpperandToLowerbenchmarks, includingBenchmark_StdToUpperandBenchmark_StdToLower. This addition enables the Go benchmark tool to report the throughput in bytes per second, offering a more comprehensive view of the functions' performance characteristics.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #123 +/- ##
=======================================
Coverage 96.36% 96.36%
=======================================
Files 9 9
Lines 495 495
=======================================
Hits 477 477
Misses 12 12
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request refactors the benchmarks for ToUpper and ToLower to use the standard b.N loop and b.SetBytes, which provides more accurate and informative performance metrics. The changes are a definite improvement over the previous implementation.
My main feedback is to remove the redundant allocation checks from the benchmark functions. These checks are already present in the corresponding Test functions, which is the more appropriate place for correctness guarantees. This will improve the separation of concerns between tests and benchmarks.
Otherwise, the changes look good.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
strings_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-07-01T15:48:53.094Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
strings_test.go (9)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/datasources/database/db_mock.go:13-19
Timestamp: 2024-11-23T19:50:06.387Z
Learning: In test code within `clean-code/app/datasources/database/db_mock.go`, adding safety checks like context validation, safe type assertions, and extra documentation is not necessary.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-07-01T15:48:53.094Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
🧬 Code Graph Analysis (1)
strings_test.go (1)
strings.go (2)
ToUpper(30-50)ToLower(8-27)
🪛 GitHub Check: lint
strings_test.go
[failure] 230-230:
unnecessary trailing newline (whitespace)
🪛 GitHub Actions: golangci-lint
strings_test.go
[error] 220-220: golangci-lint (revive): extra empty line at the end of a block (empty-lines)
🪛 GitHub Actions: Test
strings_test.go
[error] 169-169: Test failure in Test_ToUpper/numbers: Expected zero allocations but got 87. ToUpper should not allocate for numbers.
🔇 Additional comments (5)
strings_test.go (5)
222-223: Excellent benchmark improvements!Adding
b.SetBytes()andb.ResetTimer()follows Go benchmarking best practices by enabling throughput metrics and excluding setup time from measurements.
225-228: Great refactoring to separate allocation measurement from performance measurement.The change from
testing.AllocsPerRunto a direct loop overb.Naligns perfectly with the PR objectives. Moving the correctness check outside the loop ensures it doesn't interfere with performance measurements while still validating the benchmark results.
238-244: Consistent improvements across all benchmark functions.The same beneficial changes are applied to
Benchmark_ToLower- adding byte count reporting, timer reset, and moving correctness validation outside the performance loop.
253-254: Standard library benchmarks properly aligned.Good to see the standard library benchmarks (
Benchmark_StdToUpperandBenchmark_StdToLower) also receiving the sameb.SetBytes()andb.ResetTimer()improvements for consistent benchmark methodology.
268-269: Complete benchmark suite consistency.All benchmark functions now follow the same improved pattern for accurate performance measurement and throughput reporting.
|
Update is to benchmarks, failing benchmarks expected, ready to merge unless there are requested changes. |
Enhance benchmarks by separating allocation checks from main loops and adding input size reporting for throughput metrics.
Summary by CodeRabbit