♻️ Refactor: Improve Performance of getSplicedStrList #3318
Conversation
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v3
cpu: AMD EPYC 7763 64-Core Processor
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
_Utils_GetSplicedStrList-4 66.12n ± 1% 51.05n ± 1% -22.79% (p=0.000 n=50)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
_Utils_GetSplicedStrList-4 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=50) ¹
¹ all samples are equal
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
_Utils_GetSplicedStrList-4 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=50) ¹
¹ all samples are equal
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3318 +/- ##
==========================================
+ Coverage 84.21% 84.25% +0.03%
==========================================
Files 116 116
Lines 11558 11554 -4
==========================================
+ Hits 9734 9735 +1
+ Misses 1395 1391 -4
+ Partials 429 428 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
WalkthroughThe pull request refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant F as getSplicedStrList
T->>F: Call with headerValue string
loop Process each character
F->>F: Evaluate character via switch
alt Comma encountered
F->>F: Append current substring to result
end
end
F->>T: Return resulting string slice
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
helpers.go (1)
326-328: Consider using more descriptive variable names.While the code is efficient, the variable names could be more descriptive:
rcould bestartorsegmentStartleadingcould beisLeadingSpace-r := 0 -leading := true +segmentStart := 0 +isLeadingSpace := true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helpers.go(1 hunks)helpers_test.go(1 hunks)
🔇 Additional comments (2)
helpers_test.go (1)
306-325: Great addition of comprehensive test cases!The new test cases effectively cover important edge cases for string splitting:
- Handling spaces between words
- Single and multiple comma scenarios
- Combinations of commas and spaces
These tests help ensure the robustness of the refactored
getSplicedStrListfunction.helpers.go (1)
326-342: Excellent refactoring for improved performance and readability!The refactored implementation:
- Eliminates the need for
utils.TrimLeftby tracking leading spaces directly- Reduces memory allocations by using string slicing
- Improves readability with a clear switch-based state machine
- Matches the reported 22.79% performance improvement
The PR objectives are well met with this implementation.
|
I'm not sure whether the failed test was caused by my changes or not. @ReneWerner87 Could you help verify this?
|
the run was really flaky, its not related to your changes |
Description
Improve the implementation of
getSplicedStrList.getSplicedStrListcallsutils.TrimLeft. In the new one, I integrated the trim-left functionality by tracking the leading space, without calling other functions.getSplicedStrListcopies thedstslice when it is not large enough. I believe this operation can be handled more efficiently using golang built-inappendfunction.Type of change
Performance improvement (non-breaking change which improves efficiency)
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.