-
-
Notifications
You must be signed in to change notification settings - Fork 37
Add backtracking #149
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
base: main
Are you sure you want to change the base?
Add backtracking #149
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
- Coverage 90.43% 84.92% -5.52%
==========================================
Files 5 5
Lines 251 325 +74
==========================================
+ Hits 227 276 +49
- Misses 24 49 +25
🚀 New features to boost your workflow:
|
gwynne
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.
My nits are not particularly critical, not using them will not make a huge difference. It'd be interesting to know if they benchmarked differently though.
| if let partials = currentNode.partials, !partials.isEmpty { | ||
| for partial in partials { | ||
| alternatives.append( | ||
| .init( | ||
| node: partial.node, | ||
| index: index, | ||
| kind: .partial(partial), | ||
| parameterSnapshot: parameters | ||
| )) | ||
| } | ||
| } |
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.
| if let partials = currentNode.partials, !partials.isEmpty { | |
| for partial in partials { | |
| alternatives.append( | |
| .init( | |
| node: partial.node, | |
| index: index, | |
| kind: .partial(partial), | |
| parameterSnapshot: parameters | |
| )) | |
| } | |
| } | |
| alternatives.append(contentsOf: currentNode.partials?.map { .init( | |
| node: partial.node, index: index, kind: .partial(partial), parameterSnapshot: parameters | |
| ) } ?? []) |
Note
.append(contentsOf: []) and [].map(...) both compile to no-ops, so eliding the conditional does not degrade performance.
| if let partials = currentNode.partials, !partials.isEmpty { | ||
| for partial in partials { | ||
| alternatives.append( | ||
| .init( | ||
| node: partial.node, | ||
| index: index, | ||
| kind: .partial(partial), | ||
| parameterSnapshot: parameters | ||
| )) | ||
| } | ||
| } |
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.
| if let partials = currentNode.partials, !partials.isEmpty { | |
| for partial in partials { | |
| alternatives.append( | |
| .init( | |
| node: partial.node, | |
| index: index, | |
| kind: .partial(partial), | |
| parameterSnapshot: parameters | |
| )) | |
| } | |
| } | |
| alternatives.append(contentsOf: currentNode.partials?.map { .init( | |
| node: partial.node, index: index, kind: .partial(partial), parameterSnapshot: parameters | |
| ) } ?? []) |
Note
.append(contentsOf: []) and [].map(...) both compile to no-ops, so eliding the conditional does not degrade performance.
Benchmark Report for dda4c9b❓ Pull request has significant performance differences 📊 Click to expand comparison resultBenchmark check running at 2025-11-25 11:29:11 UTCReading thresholds from "Benchmarks/Thresholds/" Checking ["RouterPerformance:Case-sensitive", "RouterPerformance:Case-insensitive", "RouterPerformance:Case-insensitive_Match_First", "RouterPerformance:Case-insensitive_Match_Last", "RouterPerformance:Case-sensitive_Minimal", "RouterPerformance:Case-insensitive_Minimal", "RouterPerformance:Minimal_Early_Fail"]
The baseline 'Add backtracking' is WORSE than the defined thresholds. Click to expand benchmark resultBaseline 'Add backtracking'RouterPerformanceCase-insensitive
Case-insensitive_Match_First
Case-insensitive_Match_Last
Case-insensitive_Minimal
Case-sensitive
Case-sensitive_Minimal
Minimal_Early_Fail
|
Fixes #145
This adds recursive backtracking to the trie algorithm. Unfortunately this will likely result in some performance loss when routing but we do want routes to be resolved correctly