refactor: No normalized path interning - #375
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 92.74% 92.63% -0.12%
==========================================
Files 110 110
Lines 4687 4736 +49
Branches 2373 2391 +18
==========================================
+ Hits 4347 4387 +40
- Misses 340 349 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NormalizedPath drops the interned cache and hence the caching strategy can no longer be set via the static NormalizedPath.interned property. Cache (and LruCache/NoCache) stay for static_handler.
- fromUri reuses the unmodifiable Uri.pathSegments list when the path needs no normalization. - Precompute each parameter Symbol at registration instead of per lookup. - _lookupRecursive threads one mutable params map with undo on backtrack instead of copying it at each level.
PathTrie tracks needsBacktracking: true when a node has both literal children and a dynamic segment. lookup uses the fast non-backtracking walk unless the table needs backtracking; backtrack: false lets a literal shadow an overlapping parameter. _lookupIterative is now a live default path.
4365a02 to
c4cba4d
Compare
| parameters[dynamicSegment.symbol] = segment; | ||
| final result = next(dynamicNode, newMap); | ||
| if (result != null) return result; | ||
| parameters.remove(dynamicSegment.symbol); // backtrack |
There was a problem hiding this comment.
Looks like this drops the outer binding when the same param name is bound at two levels and the inner branch fails:
final trie = PathTrie<int>()
..add(NormalizedPath('/:x/lit/:x/a'), 1)
..add(NormalizedPath('/:x/:y/c'), 2);
trie.lookup(NormalizedPath('/v/lit/c')); // params = {y: lit} — x is goneOn main this returns {x: v, y: lit} since the per-level copy kept the outer value. Probably a good place to add a test as well.
| } | ||
| currentNode.children.addAll(node.children); | ||
|
|
||
| _needsBacktracking = |
There was a problem hiding this comment.
I ran into what looks like a stale-flag case with group(). Since attach(consume: false) shares the subtree, routes added to the sub router afterwards mutate the parent's nodes but only seem to set the sub trie's flag:
final sub = router.group('/api');
sub.get('/users/x', 1);
sub.get('/users/:id/y', 2);
router.lookup(Method.get, '/api/users/x/y'); // PathMiss — matches {id: x} on maininjectAt looks fine since it consumes. Maybe the flag needs to live with the shared structure rather than the PathTrie wrapper? Not sure what the cleanest fix is. Could use a regression test too, the backtrack tests cover the happy paths but not this one.
| /// ``` | ||
| /// A no-op [Cache] that never stores or retrieves values, for opting out of | ||
| /// caching in high-cardinality workloads where it costs more than it saves. | ||
| final class NoCache<K, V> implements Cache<K, V> { |
There was a problem hiding this comment.
Is NoCache still needed? I couldn't find any consumers now that NormalizedPath.interned is gone, so there doesn't seem to be anything to plug it into.
|
|
||
| group('Given NormalizedPath with NoCache', () { | ||
| late Cache<String, NormalizedPath> originalCache; | ||
| group('Given a Router (no result cache)', () { |
There was a problem hiding this comment.
nit: this group tests Router.lookupUri rather than NoCache, maybe better off in a router test file?
2 participants
Description
Remove NormalizedPath interning
This PR removes NormalizedPath interning completely.
Interning was originally added to avoid re-normalizing paths, but it always had two problems:
On top of that, the new
NormalizedPath.fromSegmentsandNormalizedPath.fromUriconstructors - now the ones actually used on the request path - made interning tricky, so the security PRs (#369 & #371) simply skipped it for those paths.Most of the lost performance is recouped by:
Results
Fixes: #118 (not by improving caching, but by dropping it)
Fixes: #342 (this time by disabling interning altogether, instead of making caching opt-out)
Closes: #344 (investigation done)
Fixes: #374
Breaking changes
NormalizedPath.internedis removed