-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
🚀 perf: Inline Request state wrappers #3827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…est instantiation and ~2.5% less GC scan bytes) Signed-off-by: Artur Melanchyk <[email protected]>
|
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. WalkthroughConverts several Request struct fields from pointer types to value types and updates initialization and access accordingly. Adds a new benchmark measuring GC heap/total scan bytes during batched Request Acquire/Release. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 #3827 +/- ##
==========================================
+ Coverage 92.20% 92.24% +0.04%
==========================================
Files 115 115
Lines 9671 9671
==========================================
+ Hits 8917 8921 +4
+ Misses 482 478 -4
Partials 272 272
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:
|
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/request.go (1)
327-327: Optional: Remove unnecessary parentheses.The parentheses around
r.cookiesandr.pathare redundant since these are now value-type maps. Direct indexing works without them:- if val, ok := (r.cookies)[key]; ok { + if val, ok := r.cookies[key]; ok {- if val, ok := (r.path)[key]; ok { + if val, ok := r.path[key]; ok {Also applies to: 366-366
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/hooks_test.go(1 hunks)client/request.go(4 hunks)client/request_bench_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:
client/request.goclient/hooks_test.goclient/request_bench_test.go
🧬 Code graph analysis (3)
client/request.go (1)
client/client.go (1)
Client(37-67)
client/hooks_test.go (1)
client/request.go (1)
FormData(874-876)
client/request_bench_test.go (1)
client/request.go (3)
Request(46-73)AcquireRequest(983-989)ReleaseRequest(993-996)
🔇 Additional comments (4)
client/request.go (2)
50-57: LGTM! Core refactor correctly reduces pointer indirection.Converting these fields from pointers to value types reduces GC scan pressure for pooled Request objects while maintaining the public API. The approach aligns well with the PR's performance goals.
970-976: Run betteralign locally to verify struct field alignment.The initialization code (lines 970-976) correctly uses value types. However, betteralign is not available in the sandbox environment. Per coding guidelines, you must run the following on your local machine to verify optimal struct field alignment:
betteralign -test_files=false ./clientReview the output for the
Requeststruct to ensure fields are optimally ordered to minimize memory padding.client/hooks_test.go (1)
796-796: LGTM! Test helper correctly updated.The formData initialization properly uses the value type FormData, consistent with the Request struct refactoring.
client/request_bench_test.go (1)
12-57: LGTM! Benchmark correctly measures heap scan impact.The benchmark design is sound:
- Explicit GC calls (acknowledged with revive comments) are necessary to capture scan metrics
- Timer isolation correctly measures only request creation time
- Metrics are reported per-iteration (batch of 512 requests), which aligns with the cumulative scan-bytes values mentioned in the PR objectives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes memory allocation by changing several fields in the Request struct from pointer types to value types, reducing the number of heap allocations per request. Additionally, a new benchmark test is introduced to measure GC heap scan overhead.
- Changed
Requeststruct fields from pointer to value types:header,params,cookies,path, andformData - Added
BenchmarkRequestHeapScanto measure GC scanning overhead during request allocation and release - Updated pool initialization and test helpers to reflect the new non-pointer field types
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| client/request.go | Changed five Request struct fields from pointer to value types and updated initialization in the request pool |
| client/request_bench_test.go | Added new benchmark to measure heap scan metrics for request batches |
| client/hooks_test.go | Updated test helper to use value type instead of pointer for formData field |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
gaby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
got similar results on VPS with 2x dedicated |
Description
This PR drops unneeded pointers in the
Request. This change doesn't affectRequest's public API.As a result GC now needs to scan 2.5% less bytes, also now new
Requestinstantiation is 4% fasterChanges 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