🩹 Fix: Optimize Cache middleware handler #3031
Conversation
orvillesimba
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe recent changes in Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/cache/cache.go (2 hunks)
Additional context used
golangci-lint
middleware/cache/cache.go
[warning] 109-109: unhandled-error: Unhandled error in call to function strings.Builder.WriteString (revive)
[warning] 110-110: unhandled-error: Unhandled error in call to function strings.Builder.WriteString (revive)
[warning] 111-111: unhandled-error: Unhandled error in call to function strings.Builder.WriteString (revive)
Additional comments not posted (2)
middleware/cache/cache.go (2)
99-99: Optimization by caching the request method.Caching the result of
c.Method()inrequestMethodis a good practice to avoid redundant method calls, especially in performance-critical middleware.
102-102: Use ofslices.Containsfor method checking.Replacing manual iteration with
slices.Containssimplifies the code and potentially improves performance by leveraging optimized library functions.
|
@orvillesimba can you please make another benchmark and post before and after in the upper one the numbers have gotten worse |
Whoops, sorry I've mistaken ns/op, I've reverted using After changes: Now we only benefit from "requestMethod := c.Method() to avoid call Method() multiple times" 😅 |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/cache/cache.go (2 hunks)
Additional context used
golangci-lint
Additional comments not posted (2)
middleware/cache/cache.go (2)
99-102: Optimization usingslices.Containsfor method checking.This change simplifies the method check and potentially improves performance by using Go's optimized slice operations. Good use of modern Go features!
99-109: Performance concerns with the new changes.The benchmark results indicate an increase in execution time and memory allocations. While the code optimizations are beneficial, the performance impact should be reconsidered. It might be useful to explore why the new approach increases resource usage and whether there are ways to mitigate this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3031 +/- ##
==========================================
- Coverage 82.86% 82.78% -0.08%
==========================================
Files 115 115
Lines 8230 8227 -3
==========================================
- Hits 6820 6811 -9
- Misses 1082 1086 +4
- Partials 328 330 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gaby
changed the title
There was a problem hiding this comment.
LGTM!
~/Documents/GitHub/fiber (main*) » benchstat before.txt after.txt sixcolors@Jason-McNeils-Mac-Pro-Monterey
goos: darwin
goarch: amd64
pkg: github.com/gofiber/fiber/v3/middleware/cache
cpu: Intel(R) Xeon(R) CPU X5675 @ 3.07GHz
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
_Cache-24 550.7n ± 1% 545.8n ± 1% -0.89% (p=0.034 n=10)
│ before.txt │ after.txt │
│ B/op │ B/op vs base │
_Cache-24 16.00 ± 0% 16.00 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
│ before.txt │ after.txt │
│ allocs/op │ allocs/op vs base │
_Cache-24 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
|
🥳 |
Description
This PR optimize cache middleware's performace, with the following changes:
slices.Containsto replace original for loop (This should not increase performance but only reduce code)strings.Builderfor concat strings forkeyrequestMethod := c.Method()to avoid callMethod()multiple timesBenchmark:
before
after
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