🧹chore: benchmark for cache miss case#3836
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new benchmark function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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. Comment |
Summary of ChangesHello @arturmelanchyk, 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 enhances the testing suite for the cache middleware by adding a dedicated benchmark for cache miss scenarios. This new benchmark will help in accurately measuring the performance overhead when the cache needs to generate and store new data, ensuring a more comprehensive understanding of the middleware's efficiency under various conditions. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
There was a problem hiding this comment.
Code Review
This pull request adds a benchmark for cache misses in the cache middleware. However, the current implementation of Benchmark_Cache_Miss is flawed, as it primarily measures cache hits instead of misses after the first iteration. I've provided a review comment with a suggested fix to ensure that each request in the benchmark loop results in a cache miss, providing an accurate measurement. This change will make the new benchmark correctly fulfill its intended purpose.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3836 +/- ##
==========================================
- Coverage 92.26% 92.19% -0.07%
==========================================
Files 115 115
Lines 9742 9742
==========================================
- Hits 8988 8982 -6
- Misses 480 485 +5
- Partials 274 275 +1
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/cache/cache_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
middleware/cache/cache_test.go
🧬 Code graph analysis (1)
middleware/cache/cache_test.go (3)
internal/storage/memory/memory.go (1)
New(29-45)app.go (2)
New(521-634)Handler(40-40)middleware/cache/cache.go (1)
New(70-374)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: Compare
9c5c0e0 to
5ba48e4
Compare
Signed-off-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
5ba48e4 to
3441187
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/cache/cache_test.go (1)
1628-1653: Consider usingfor b.Loop()for consistency with other benchmarks.The cache miss logic is correctly implemented—varying the URI ensures each iteration misses the cache. However, all other benchmarks in this file use the newer
for b.Loop()pattern (lines 1620, 1676, 1703, 1734), while this benchmark uses the traditionalfor n := 0; n < b.N; n++style.Apply this diff for consistency:
func Benchmark_Cache_Miss(b *testing.B) { app := fiber.New() app.Use(New()) app.Get("/*", func(c fiber.Ctx) error { data, _ := os.ReadFile("../../.github/README.md") //nolint:errcheck // We're inside a benchmark return c.Status(fiber.StatusOK).Send(data) }) h := app.Handler() fctx := &fasthttp.RequestCtx{} fctx.Request.Header.SetMethod(fiber.MethodGet) b.ReportAllocs() b.ResetTimer() - for n := 0; n < b.N; n++ { + var n int + for b.Loop() { + n++ fctx.Request.SetRequestURI("/demo/" + strconv.Itoa(n)) h(fctx) } require.Equal(b, fiber.StatusOK, fctx.Response.Header.StatusCode()) require.Greater(b, len(fctx.Response.Body()), 30000) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/cache/cache_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
middleware/cache/cache_test.go
🧠 Learnings (1)
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/cache/cache_test.go
🧬 Code graph analysis (1)
middleware/cache/cache_test.go (2)
internal/storage/memory/memory.go (1)
New(29-45)middleware/cache/cache.go (1)
New(70-374)
|
@arturmelanchyk thx, can you check the hint from the AI and my comment |
Signed-off-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
|
@ReneWerner87 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/cache/cache_test.go (1)
1628-1655: LGTM! Correctly implements cache miss benchmark.The implementation properly addresses previous review feedback by varying the request URI on each iteration (lines 1648-1649), ensuring every call results in a cache miss. The use of
b.ResetTimer()after setup is correct, and the wildcard route/*appropriately matches all unique paths.Optional consistency improvements:
- Consider adding a benchmark invocation comment for consistency with other benchmarks:
+// go test -v -run=^$ -bench=Benchmark_Cache_Miss -benchmem -count=4 func Benchmark_Cache_Miss(b *testing.B) {
- For easier performance comparison with
Benchmark_Cache, consider using the same status code:- return c.Status(fiber.StatusOK).Send(data) + return c.Status(fiber.StatusTeapot).Send(data)And update the assertion:
- require.Equal(b, fiber.StatusOK, fctx.Response.Header.StatusCode()) + require.Equal(b, fiber.StatusTeapot, fctx.Response.Header.StatusCode())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/cache/cache_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
middleware/cache/cache_test.go
🧠 Learnings (2)
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2025-09-18T00:32:59.671Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2025-09-18T00:32:59.671Z
Learning: In Go 1.22, manual copying of loop variables (like `x := x`) is no longer necessary due to the fix for variable capture in closures. Each loop iteration now creates its own instance of the loop variable automatically.
Applied to files:
middleware/cache/cache_test.go
🧬 Code graph analysis (1)
middleware/cache/cache_test.go (1)
middleware/cache/cache.go (1)
New(70-374)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: Compare
|
coderabbit's comment regarding |
Description
Existing benchmarks for cache middlware seems to not cover case with cache miss, when the new entry needs to be generated
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md