♻️ Refactor: the value of map is unused in uniqueRouteStack #3320
Conversation
WalkthroughThe changes refactor the internal handling of unique routes within a function. The map used for tracking route uniqueness now stores empty struct values instead of integers that recorded positions. This simplifies the logic and removes unnecessary data without affecting the external behavior of the function. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
✨ 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 (
|
|
can you share benchmarks ? is it a speed improvement |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3320 +/- ##
==========================================
- Coverage 84.22% 84.21% -0.01%
==========================================
Files 116 116
Lines 11560 11558 -2
==========================================
- Hits 9736 9734 -2
Misses 1395 1395
Partials 429 429
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The main purpose is to improve code readability because the original approach stored an integer in the map without ever reading it, which is quite unreasonable. I think this PR has negligible benefit in terms of performance. func Benchmark_Utils_UniqueRouteStack(b *testing.B) {
for i := 0; i < b.N; i++ {
route1 := &Route{}
route2 := &Route{}
route3 := &Route{}
require.Equal(
b,
[]*Route{
route1,
route2,
route3,
},
uniqueRouteStack([]*Route{
route1,
route1,
route1,
route2,
route2,
route2,
route3,
route3,
route3,
route1,
route2,
route3,
}))
}
} |
|
@ReneWerner87 Benchmark for large data This benchmark tests the performance of inserting 16 different func Benchmark_Utils_UniqueRouteStack(b *testing.B) {
routers := make([]Route, 16)
for i := 0; i < b.N; i++ {
uniqueRouteStack([]*Route{
&routers[0],
&routers[0],
&routers[1],
&routers[1],
&routers[2],
&routers[2],
&routers[3],
&routers[3],
&routers[4],
&routers[4],
&routers[5],
&routers[5],
&routers[6],
&routers[6],
&routers[7],
&routers[7],
&routers[8],
&routers[8],
&routers[9],
&routers[9],
&routers[10],
&routers[10],
&routers[11],
&routers[11],
&routers[12],
&routers[12],
&routers[13],
&routers[13],
&routers[14],
&routers[14],
&routers[15],
&routers[15],
})
}
}Benchmark Results:
|
Description
The value of map
minuniqueRouteStackis unused. We can set the value type ofmtostruct{}instead of int to save some memory and improve code clarity and readability.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.