🧹 chore: add parallel benchmark variants for router and app#4042
🧹 chore: add parallel benchmark variants for router and app#4042ReneWerner87 merged 8 commits intomainfrom
Conversation
Summary of ChangesHello @gaby, 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 significantly expands the project's benchmarking capabilities by introducing parallel execution for a wide array of tests related to application context, routing, and general application functionality. The primary goal is to gain more accurate performance insights under concurrent conditions, which is crucial for identifying bottlenecks and ensuring stability in high-load scenarios. Additionally, the PR refactors common setup logic for benchmarks into reusable helper functions, promoting cleaner and more maintainable test code. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
WalkthroughAdds parallel benchmarks and benchmark-only helpers across Changes
Sequence Diagram(s)(omitted — changes are benchmark additions and helpers; no new runtime control-flow feature requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4042 +/- ##
==========================================
- Coverage 91.21% 91.15% -0.06%
==========================================
Files 119 119
Lines 11117 11117
==========================================
- Hits 10140 10134 -6
- Misses 620 625 +5
- Partials 357 358 +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.
Pull request overview
Adds parallel benchmark variants to better measure hot paths under concurrent load, plus small helpers to make benchmark ctx acquisition/type assertions more consistent.
Changes:
- Added per-file benchmark helpers to acquire
*DefaultCtxfromApp.AcquireCtxwith a fatal-on-mismatch assertion. - Added many new
*_Parallelbenchmarks usingb.RunParallel()inctx_test.goandrouter_test.go. - Added parallel variants for a couple of app-level benchmarks in
app_test.go.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
router_test.go |
Adds acquireDefaultCtxForRouterBenchmark and many new router/app parallel benchmarks. |
ctx_test.go |
Adds acquireDefaultCtxForBenchmark and a large set of new ctx method parallel benchmarks. |
app_test.go |
Adds parallel benchmark variants and acquireDefaultCtxForAppBenchmark helper. |
| appHandler := app.Handler() | ||
| c := &fasthttp.RequestCtx{} | ||
| c.Request.Header.SetMethod("DELETE") | ||
| c.URI().SetPath("/this/is/a/dummy/route/oke") | ||
| b.RunParallel(func(pb *testing.PB) { | ||
| for pb.Next() { | ||
| appHandler(c) | ||
| } | ||
| }) |
There was a problem hiding this comment.
This parallel benchmark shares a single *fasthttp.RequestCtx (c) across all goroutines. b.RunParallel executes concurrently, so the handler will read/write the same request/response buffers at the same time, causing data races and invalid results. Allocate/configure a separate RequestCtx per worker (inside the RunParallel closure) and avoid asserting on a concurrently-mutated response object.
router_test.go
Outdated
| var match bool | ||
| var params [maxParams]string | ||
| parsed := parseRoute("/user/keys/:id") | ||
| route := &Route{use: false, root: false, star: false, routeParser: parsed, Params: parsed.params, path: "/user/keys/:id", Path: "/user/keys/:id", Method: "DELETE"} | ||
| route.Handlers = append(route.Handlers, func(_ Ctx) error { | ||
| return nil | ||
| }) | ||
| b.RunParallel(func(pb *testing.PB) { | ||
| for pb.Next() { | ||
| match = route.match("/user/keys/1337", "/user/keys/1337", ¶ms) | ||
| } | ||
| }) |
There was a problem hiding this comment.
route.match writes into params and returns match, but both params and match are shared across all parallel workers here. That introduces data races and makes the post-benchmark assertions nondeterministic. Move params/match to worker-local variables inside the RunParallel closure (and only assert using a single-threaded verification run).
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@ctx_test.go`:
- Around line 8511-8544: Benchmarks like Benchmark_Ctx_Accepts_Parallel and
Benchmark_Ctx_AcceptsCharsets_Parallel currently share a single *DefaultCtx /
*fasthttp.RequestCtx and a shared result variable across goroutines, causing
data races; fix by acquiring a fresh ctx per goroutine (inside the RunParallel
loop or per worker) using app.AcquireCtx or acquireDefaultCtxForBenchmark, keep
result variables (e.g., res) local to the goroutine, call c.Accepts /
c.AcceptsCharsets on that per-goroutine ctx, and release it after use (e.g.,
app.ReleaseCtx(c) or the appropriate teardown) so no shared mutable ctx/state is
accessed concurrently.
In `@router_test.go`:
- Around line 2187-2202: Benchmark_Route_Match_Parallel currently shares the
mutable variables match and params across goroutines causing data races; fix by
making match and params local to each worker inside the b.RunParallel closure
(e.g., declare local vars like matchLocal and paramsLocal inside the pb loop and
call route.match with ¶msLocal), then after the parallel section perform a
single non-parallel invocation to assert correctness using parsed.params (or run
one sequential check using route.match and a fresh params slice) so benchmarks
remain race-free; apply the same change to the other two similar benchmarks that
use params/match.
- Around line 2018-2028: Benchmark_App_RebuildTree_Parallel races because it
mutates shared App state (app.routesRefreshed and app.RebuildTree) concurrently;
either create a fresh App per parallel worker inside b.RunParallel (instantiate
New() and registerDummyRoutes for each goroutine) or serialize access by
protecting the mutation and call to RebuildTree with a sync.Mutex around the
critical section (use a package-local mutex referenced in the benchmark),
ensuring Benchmark_App_RebuildTree_Parallel no longer mutates the same App
concurrently.
- Around line 2149-2168: Benchmark_Router_Next_Parallel currently shares a
single DefaultCtx (c) and result vars across goroutines; move the context
acquisition into the RunParallel closure by calling
acquireDefaultCtxForRouterBenchmark(b, app, request) (or creating a fresh
RequestCtx/Request) inside the b.RunParallel func so each goroutine gets its own
DefaultCtx, make res and err local to the goroutine, and then after
b.RunParallel perform a single non-parallel check (create one DefaultCtx and
call app.next) to assert require.NoError and require.True and verify
c.indexRoute equals 4.
- Around line 2031-2050: The benchmark reuses a single fasthttp.RequestCtx
across goroutines in Benchmark_App_MethodNotAllowed_Parallel (and the other
listed benchmarks), causing data races; change each benchmark to allocate and
initialize a new *fasthttp.RequestCtx inside the b.RunParallel loop (or inside
the pb.Next iteration) rather than reusing the shared c variable — follow the
same per-iteration allocation/initialization pattern used in
Benchmark_Router_Next_Default_Parallel so each parallel worker constructs its
own RequestCtx, sets method and URI, and then calls appHandler(c).
- Around line 2287-2307: Benchmark_Router_GitHub_API_Parallel has a data race
because c, match, and err are shared across goroutines; move the
fasthttp.RequestCtx variable c and the locals match and err inside the
b.RunParallel closure so each goroutine gets its own instances, set the request
method and URI inside the closure before calling
acquireDefaultCtxForRouterBenchmark, call app.next and app.ReleaseCtx with those
goroutine-local vars, and remove the require.NoError and require.True assertions
at the end of the benchmark since parallel benchmark iterations shouldn't
perform correctness checks; refer to symbols
Benchmark_Router_GitHub_API_Parallel, c, match, err,
acquireDefaultCtxForRouterBenchmark, app.next, app.ReleaseCtx, and
routesFixture.TestRoutes to locate where to change.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ctx_test.go (1)
8448-8479:⚠️ Potential issue | 🟠 MajorResolve unused benchmark helper to clear lint failure.
acquireDefaultCtxForBenchmarkis currently unused, and golangci-lint fails because of it. Either remove it or wire it into existing benchmarks. Here’s a minimal wiring that both uses the helper and simplifies the existing type assertion.🛠️ Suggested fix (use helper in Benchmark_Ctx_OverrideParam)
func Benchmark_Ctx_OverrideParam(b *testing.B) { app := New() - ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) - c, ok := ctx.(*DefaultCtx) - if !ok { - b.Fatal("AcquireCtx did not return *DefaultCtx") - } - - defer app.ReleaseCtx(c) + c := acquireDefaultCtxForBenchmark(b, app, &fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) c.values = [maxParams]string{"original", "12345"} c.route = &Route{Params: []string{"name", "id"}} c.setMatched(true) @@ for b.Loop() { c.OverrideParam("name", "changed") } }
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@router_test.go`:
- Around line 2177-2192: The benchmark shares a single fasthttp.RequestCtx
(fctx) across goroutines causing a data race; inside
Benchmark_Router_Next_Default_Immutable_Parallel move creation and setup of the
request context into the b.RunParallel closure (or into the per-goroutine loop)
so each worker uses its own local fasthttp.RequestCtx, e.g. create a local fctx,
set MethodGet and SetRequestURI("/") before calling h(fctx) in the pb.Next loop
to eliminate shared state.
- Around line 2052-2070: Benchmark_Router_NotFound_Parallel is using a single
shared fasthttp.RequestCtx (c) across goroutines causing a data race; move the
RequestCtx creation and request setup inside the b.RunParallel closure so each
goroutine/iteration uses its own local c. Specifically, in
Benchmark_Router_NotFound_Parallel, allocate a new c := &fasthttp.RequestCtx{}
inside the pb loop, set c.Request.Header.SetMethod("DELETE") and
c.URI().SetPath("/this/route/does/not/exist") there, then call appHandler(c);
keep appHandler (from New()/registerDummyRoutes) as-is. Ensure any assertions
that read c.Response are done on per-iteration locals or removed/adjusted so
they don't reference a shared ctx after RunParallel.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
🐛 fix: eliminate data races in parallel router benchmarks
Motivation
AcquireCtxplus type assertions for benchmarks to make parallel benchmark bodies safer and clearer.Description
acquireDefaultCtxForBenchmarktoctx_test.go,acquireDefaultCtxForRouterBenchmarktorouter_test.go, andacquireDefaultCtxForAppBenchmarktoapp_test.goto centralizeAcquireCtx+*DefaultCtxassertion and failure handling for benchmarks._Parallelvariants that useb.RunParallel()for many existing benchmarks inctx_test.go,router_test.go, andapp_test.go(examples:Benchmark_NewError_Parallel,Benchmark_Communication_Flow_Parallel,Benchmark_Ctx_AcquireReleaseFlow_Parallel, and manyCtx_*_Paralleland router benchmark parallels).Fixes #3048