-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
♻️ Refactor: reduce DefaultCtx from 768 bytes to 736 bytes #3353
Conversation
WalkthroughThis pull request refactors the handling of HTTP paths by updating the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Ctx as DefaultCtx
participant Utils
participant Match as match()
App->>Ctx: Process request context
Ctx-->>App: Return modified path via getString()
App->>Utils: Convert c.detectionPath to UnsafeString
App->>Utils: Convert c.path to UnsafeString
App->>Match: Call match(UnsafeString(detectionPath), UnsafeString(path))
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3353 +/- ##
==========================================
+ Coverage 83.68% 83.73% +0.05%
==========================================
Files 118 118
Lines 11728 11722 -6
==========================================
+ Hits 9814 9816 +2
+ Misses 1486 1480 -6
+ Partials 428 426 -2
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one comment.
I will check it later today and probably merge |
I am still unsure, at that time the additional buffers were necessary to reduce the allocations and preserve the original |
I reduce the two string fields in the |
yes already understood, just want to make sure that we don't have any performance losses(more allocs) or that the strings are not changed by the referencing I will test this by hand |
really good work, I checked everything again and we also have tests for the cases where I suspected that something could still go wrong https://github.com/ksw2000/fiber/blob/395c8fafa96719cfcce808876c4b11914ca34efc/app_test.go#L500 thanks for your work and patience |
Description
In the default context,
pathBuffer
anddetectionPathBuffer
can be used aspath
anddetectionPath
directly. We can remove twostring
fields and save32 bytes
for each context.Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/
directory for Fiber's documentation.