-
-
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 the memory usage of RoutePatternMatch #3335
Conversation
``` goos: linux goarch: amd64 pkg: github.com/gofiber/fiber/v3 cpu: AMD EPYC 7763 64-Core Processor │ route_pattern_match_old.txt │ route_pattern_match_new.txt │ │ sec/op │ sec/op vs base │ _RoutePatternMatch//api/v1/const_|_match_|_/api/v1/const-4 263.4n ± 2% 249.0n ± 4% -5.47% (p=0.001 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1-4 258.7n ± 4% 244.7n ± 2% -5.43% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/-4 254.6n ± 4% 246.3n ± 2% -3.26% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/something-4 265.1n ± 4% 255.6n ± 3% -3.60% (p=0.001 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_match_|_/api/abc/fixedEnd-4 775.9n ± 3% 775.6n ± 2% ~ (p=0.424 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_not_match_|_/api/abc/def/fixedEnd-4 796.7n ± 3% 767.1n ± 2% -3.72% (p=0.001 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity-4 916.2n ± 1% 904.8n ± 3% ~ (p=0.052 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/-4 913.8n ± 4% 909.1n ± 3% ~ (p=0.393 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/1-4 915.0n ± 3% 907.2n ± 2% ~ (p=0.165 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v-4 917.5n ± 2% 876.7n ± 2% -4.46% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v2-4 918.5n ± 2% 886.8n ± 2% -3.45% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v1/-4 935.6n ± 2% 901.9n ± 2% -3.60% (p=0.000 n=10) geomean 588.3n 570.7n -2.99% │ route_pattern_match_old.txt │ route_pattern_match_new.txt │ │ B/op │ B/op vs base │ _RoutePatternMatch//api/v1/const_|_match_|_/api/v1/const-4 168.0 ± 0% 152.0 ± 0% -9.52% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1-4 160.0 ± 0% 144.0 ± 0% -10.00% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/-4 160.0 ± 0% 144.0 ± 0% -10.00% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/something-4 176.0 ± 0% 160.0 ± 0% -9.09% (p=0.000 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_match_|_/api/abc/fixedEnd-4 440.0 ± 0% 440.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/:param/fixedEnd_|_not_match_|_/api/abc/def/fixedEnd-4 464.0 ± 0% 440.0 ± 0% -5.17% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity-4 536.0 ± 0% 536.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/-4 536.0 ± 0% 536.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/1-4 536.0 ± 0% 536.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v-4 544.0 ± 0% 528.0 ± 0% -2.94% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v2-4 544.0 ± 0% 528.0 ± 0% -2.94% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v1/-4 544.0 ± 0% 528.0 ± 0% -2.94% (p=0.000 n=10) geomean 353.7 337.9 -4.47% ¹ all samples are equal │ route_pattern_match_old.txt │ route_pattern_match_new.txt │ │ allocs/op │ allocs/op vs base │ _RoutePatternMatch//api/v1/const_|_match_|_/api/v1/const-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/something-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_match_|_/api/abc/fixedEnd-4 13.00 ± 0% 13.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/:param/fixedEnd_|_not_match_|_/api/abc/def/fixedEnd-4 14.00 ± 0% 13.00 ± 0% -7.14% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity-4 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/-4 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/1-4 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v-4 15.00 ± 0% 14.00 ± 0% -6.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v2-4 15.00 ± 0% 14.00 ± 0% -6.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v1/-4 15.00 ± 0% 14.00 ± 0% -6.67% (p=0.000 n=10) geomean 10.67 9.811 -8.08% ¹ all samples are equal ```
…tPart ``` goos: linux goarch: amd64 pkg: github.com/gofiber/fiber/v3 cpu: AMD EPYC 7763 64-Core Processor │ route_pattern_match_old.txt │ route_pattern_match_new3.txt │ │ sec/op │ sec/op vs base │ _RoutePatternMatch//api/v1/const_|_match_|_/api/v1/const-4 264.3n ± 2% 253.8n ± 2% -3.95% (p=0.001 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1-4 258.5n ± 1% 247.6n ± 2% -4.24% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/-4 260.8n ± 3% 249.7n ± 4% -4.26% (p=0.003 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/something-4 265.4n ± 2% 256.1n ± 2% -3.49% (p=0.000 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_match_|_/api/abc/fixedEnd-4 783.8n ± 2% 777.5n ± 3% ~ (p=0.218 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_not_match_|_/api/abc/def/fixedEnd-4 797.8n ± 1% 773.6n ± 3% -3.03% (p=0.001 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity-4 920.3n ± 2% 926.0n ± 3% ~ (p=0.896 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/-4 920.4n ± 4% 908.2n ± 2% ~ (p=0.063 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/1-4 927.9n ± 2% 919.0n ± 3% ~ (p=0.579 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v-4 920.4n ± 3% 889.5n ± 3% -3.36% (p=0.007 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v2-4 916.9n ± 2% 891.9n ± 2% -2.73% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v1/-4 938.8n ± 5% 891.2n ± 2% -5.07% (p=0.000 n=10) geomean 591.7n 575.5n -2.73% │ route_pattern_match_old.txt │ route_pattern_match_new3.txt │ │ B/op │ B/op vs base │ _RoutePatternMatch//api/v1/const_|_match_|_/api/v1/const-4 168.0 ± 0% 152.0 ± 0% -9.52% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1-4 160.0 ± 0% 144.0 ± 0% -10.00% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/-4 160.0 ± 0% 144.0 ± 0% -10.00% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/something-4 176.0 ± 0% 160.0 ± 0% -9.09% (p=0.000 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_match_|_/api/abc/fixedEnd-4 440.0 ± 0% 440.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/:param/fixedEnd_|_not_match_|_/api/abc/def/fixedEnd-4 464.0 ± 0% 440.0 ± 0% -5.17% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity-4 536.0 ± 0% 536.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/-4 536.0 ± 0% 536.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/1-4 536.0 ± 0% 536.0 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v-4 544.0 ± 0% 528.0 ± 0% -2.94% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v2-4 544.0 ± 0% 528.0 ± 0% -2.94% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v1/-4 544.0 ± 0% 528.0 ± 0% -2.94% (p=0.000 n=10) geomean 353.7 337.9 -4.47% ¹ all samples are equal │ route_pattern_match_old.txt │ route_pattern_match_new3.txt │ │ allocs/op │ allocs/op vs base │ _RoutePatternMatch//api/v1/const_|_match_|_/api/v1/const-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/const_|_not_match_|_/api/v1/something-4 6.000 ± 0% 5.000 ± 0% -16.67% (p=0.000 n=10) _RoutePatternMatch//api/:param/fixedEnd_|_match_|_/api/abc/fixedEnd-4 13.00 ± 0% 13.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/:param/fixedEnd_|_not_match_|_/api/abc/def/fixedEnd-4 14.00 ± 0% 13.00 ± 0% -7.14% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity-4 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/-4 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_match_|_/api/v1/entity/1-4 14.00 ± 0% 14.00 ± 0% ~ (p=1.000 n=10) ¹ _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v-4 15.00 ± 0% 14.00 ± 0% -6.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v2-4 15.00 ± 0% 14.00 ± 0% -6.67% (p=0.000 n=10) _RoutePatternMatch//api/v1/:param/*_|_not_match_|_/api/v1/-4 15.00 ± 0% 14.00 ± 0% -6.67% (p=0.000 n=10) geomean 10.67 9.811 -8.08% ¹ all samples are equal ```
WalkthroughThe pull request refactors the routing logic in Changes
Sequence Diagram(s)sequenceDiagram
participant R as RoutePatternMatch
participant U as utils.ToLowerBytes
participant A as analyseParameterPart/ConstantPart
participant E as RemoveEscapeCharBytes
R->>U: Convert pattern to lowercase bytes
U-->>R: Return lowercase bytes
R->>A: Analyze pattern parts (return integer length)
A-->>R: Return processed length and segment
R->>E: Remove escape characters from byte slice
E-->>R: Return cleaned bytes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3335 +/- ##
==========================================
- Coverage 84.29% 84.23% -0.07%
==========================================
Files 116 116
Lines 11572 11578 +6
==========================================
- Hits 9755 9753 -2
- Misses 1389 1395 +6
- Partials 428 430 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 (2)
path.go (2)
155-185
: Consider consistent usage of byte slices or strings to reduce repeated conversions.You've successfully switched to byte slices for
patternPretty
but frequently convert it back to a string (lines 167, 169, 172, 185). This repeated conversion could introduce unnecessary overhead. As a good-to-have improvement, consider either consistently using byte slices throughout the matching logic or, if strings are ultimately needed, maintaining a single string variable for comparisons.
629-639
: In-place modification inRemoveEscapeCharBytes
may impact external usage of the slice.
RemoveEscapeCharBytes
performs in-place modifications on the provided slice and then slices it down. While this is efficient, be cautious if the slice is used elsewhere, as the original contents will be altered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
path.go
(7 hunks)
🔇 Additional comments (3)
path.go (3)
192-204
: Initialization of 'n' and 'seg' for route parsing looks good.Declaring and assigning
n
andseg
before entering the conditional branches simplifies readability. The logic for appending segments and parameters also appears consistent with the new return signatures.
279-291
: Returning length instead of substring is cleanly implemented.Changing
analyseConstantPart
to return just the processed length boosts performance by avoiding substring allocations. The method handles edge cases such asnextParamPosition
being-1
gracefully.
294-399
:❓ Verification inconclusive
Parameter parsing refactor looks coherent, but verify no old function calls remain.
Shifting
analyseParameterPart
to return an integer for processed length aligns with the PR’s goal of reducing allocations. Please ensure no references to the old(string, *routeSegment)
signature linger in the codebase or tests.
🏁 Script executed:
#!/bin/bash # Verify that no references to the old return signatures exist in the repository: ast-grep --pattern $'func (routeParser $_) analyseParameterPart($_) (string, $_) {\n $$$ \n}' ast-grep --pattern $'func (*routeParser) analyseConstantPart($_) (string, $_) {\n $$$ \n}'Length of output: 182
Action: Confirm No Legacy Signatures Remain
The parameter parsing refactor appears correct with the updated signature returning an integer and a pointer to a route segment. However, automated searches using ast-grep did not generate output, which means the absence of matches is inconclusive. Please manually verify that no lingering references to the old
(string, *routeSegment)
signature exist in the codebase or in tests—ensure that all call sites have been updated accordingly.
- Files to check:
path.go
(lines 294–399) and any test files that invokeanalyseParameterPart
.- Verification: Confirm that no assignments expect a
(string, *routeSegment)
return type from this function.
In lines 169, 172, and 185, the program does not allocate memory when converting between strings and byte slices. In line 167, I am still working on reducing the memory usage of |
@ksw2000 I actually ran into the same problem with |
I tried for the whole day, and I think this is a big issue that cannot be solved in one go. It needs to be optimized in multiple steps. Some functions can be refactored to be more concise. For example, |
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
Description
The string variable
patternPretty
inRoutePatternMatch
can be changed into[]byte
. The byte slice provides greater flexibility for modifying its bytes. Thus, in the benchmark test, we can observe a reduction in memory usage and allocation.RemoveEscapeCharBytes
analyseConstantPart
andanalyseParameterPart
. Because we only use the length of the returned string, we can just return the number.I'm still working on reducing memory usage in
path.go
. However, it does not always lead to a reduction in memory usage. Therefore, I am submitting this pull request first. If I succeed in further reducing memory usage and allocation times, I will submit additional pull requests.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.