performance optimizations #2947
Conversation
WalkthroughThe update focuses on refining benchmarking and context management within the application. It eliminates outdated benchmarks and introduces new ones for precise performance analysis. Context handling is streamlined by adjusting when and how the context is reset, enhancing efficiency and maintainability. Changes
Possibly related issues
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 (
|
|
ETAG bench on my machine v3(current): 82ns |
|
not finished yet, I'm still looking for the missing 20ns |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2947 +/- ##
==========================================
- Coverage 82.78% 82.76% -0.03%
==========================================
Files 116 116
Lines 8422 8411 -11
==========================================
- Hits 6972 6961 -11
Misses 1110 1110
Partials 340 340
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
this will be used to make differences between version 2 and 3 directly visible
this will be used to show differences between version 2 and 3 directly
ReneWerner87
requested review from
gaby,
sixcolors and
efectn
and removed request for
a team
|
this PR should fix the performance lost in the benchmarks | Benchmark suite | Current: 6a7f0158976cb54c2eba55ace2e1082a7d5cd60f | Previous: fd811cf84af282db8ec50adedce01a5886d5fd46 | Ratio |
|-|-|-|-|
| `Benchmark_Etag` | `200.3` ns/op 0 B/op 0 allocs/op | `97.79` ns/op 0 B/op 0 allocs/op | `2.05` |
| `Benchmark_Middleware_Favicon` | `212.9` ns/op 12 B/op 4 allocs/op | `89.1` ns/op 3 B/op 1 allocs/op | `2.39` |
| `Benchmark_Idempotency/skip` | `210.7` ns/op 0 B/op 0 allocs/op | `105` ns/op 0 B/op 0 allocs/op | `2.01` | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- app_test.go (2 hunks)
- ctx_interface.go (4 hunks)
- router.go (1 hunks)
Additional comments not posted (4)
router.go (1)
218-223: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [442-455]
Consider verifying the impact of removing
c.Reset(rctx)on context management and request handling to ensure that this optimization does not introduce any unintended side effects.ctx_interface.go (3)
442-455: The removal ofsetReqfrom theCtxinterface simplifies context management. Ensure that no functionality is adversely affected by this change.Verification successful
The search for any remaining references to
setReqin the codebase did not produce any results, indicating that the removal ofsetReqfrom theCtxinterface has been thoroughly implemented with no lingering references. This supports the initial review comment about simplifying context management through the removal ofsetReq.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to setReq in the codebase rg --type go "setReq\("Length of output: 25
442-455: Moving the initialization ofindexRoute,indexHandler,matched, andbaseURIto theResetmethod could improve efficiency. Verify that context reuse is not negatively impacted by these changes.
442-455: The adjustments to thereleasemethod align with the changes to theResetmethod. Confirm that the context is correctly reset upon release to prevent state leakage between requests.
pkg: github.com/gofiber/fiber/v3/middleware/etag
Benchmark_Etag
Benchmark_Etag-12 17534946 69.83 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 17935286 70.72 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 16889533 69.57 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 16152487 68.82 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 16907638 68.04 ns/op 0 B/op 0 allocs/oppkg: github.com/gofiber/fiber/v2/middleware/etag
Benchmark_Etag
Benchmark_Etag-12 16412640 61.38 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 20339643 59.30 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 20296197 61.29 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 18900700 63.48 ns/op 0 B/op 0 allocs/op
Benchmark_Etag-12 20087421 61.59 ns/op 0 B/op 0 allocs/oppkg: github.com/gofiber/fiber/v3/middleware/favicon
Benchmark_Middleware_Favicon
Benchmark_Middleware_Favicon-12 20613506 59.47 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 18785577 57.69 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 19518568 60.20 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 19779933 60.08 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 19941021 59.03 ns/op 3 B/op 1 allocs/oppkg: github.com/gofiber/fiber/v2/middleware/favicon
Benchmark_Middleware_Favicon
Benchmark_Middleware_Favicon-12 20951350 48.58 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 25154551 49.07 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 24965953 49.23 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 24890929 48.66 ns/op 3 B/op 1 allocs/op
Benchmark_Middleware_Favicon-12 24523046 49.49 ns/op 3 B/op 1 allocs/oppkg: github.com/gofiber/fiber/v3/middleware/idempotency
Benchmark_Idempotency
Benchmark_Idempotency/skip
Benchmark_Idempotency/skip-12 13999358 77.70 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 15908833 77.75 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 15722823 77.30 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 15650931 80.33 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 14185095 82.41 ns/op 0 B/op 0 allocs/oppkg: github.com/gofiber/fiber/v2/middleware/idempotency
Benchmark_Idempotency
Benchmark_Idempotency/skip
Benchmark_Idempotency/skip-12 16240456 61.87 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 19814664 61.26 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 18448364 62.77 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 18232786 60.44 ns/op 0 B/op 0 allocs/op
Benchmark_Idempotency/skip-12 18487230 61.32 ns/op 0 B/op 0 allocs/op |
this will be used to show differences between version 2 and 3 directly
|
somehow strange, the benchmarks still differ slightly in the middlewares etag v3 70ns vs v2 61ns think because the flow works multiple times and the 4ns from the extra feature is added up in each case |
Summary by CodeRabbit