-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
perf: use a binary search for insertMatcher #2137
Conversation
✅ Deploy Preview for vue-router canceled.
|
7255050
to
b894ef7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2137 +/- ##
==========================================
- Coverage 91.01% 90.96% -0.05%
==========================================
Files 24 24
Lines 1135 1151 +16
Branches 351 355 +4
==========================================
+ Hits 1033 1047 +14
- Misses 63 64 +1
- Partials 39 40 +1 ☔ 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.
Great PR! Thanks a lot again
I did some minor adjustments for logs
Inspired by #2132 and #2136, I had a go at speeding up
insertMatcher()
.The ordering algorithm
This PR changes
insertMatcher()
to use a binary search when trying to find the appropriate insertion point for a new matcher. This seems to give a similar performance gain to #2136, but without changing the public API for Vue Router.When I first opened this PR, I noted some assumptions I'd made about the sort order. Some of those assumptions turned out to be incorrect, so the current changes proposed here are a little different from what I had originally.
I've written a lot of extra tests to check that the sort order hasn't changed, especially in the weird edge cases. Those tests aren't included in this PR. I did open #2138 to add a couple of them, but Eduardo didn't seem keen to merge those tests. I suspect he'd be even less enthusiastic about some of the others. 😄
I believe the new sort order is equivalent to the old sort order, in the sense that the same paths will resolve to the same routes. I'm only aware of one exception to that, which I'll outline later.
The new ordering works roughly like this:
The algorithm to achieve this goes through two phases:
That second phase requires the ancestors to be in the matchers array before the children are added. For that to work, I had to move the
insertMatcher()
call inaddRoute()
to come before the recursive calls toaddRoute()
for the children.To put that another way, previously the
insertMatcher()
calls for the children would happen before theinsertMatcher()
call for the parent. This automatically led to children coming before the parent in the matchers array (assuming equal scores). I've changed the order of these calls, but the order of the matchers array should still be the same.In addition to testing performance with the benchmark from #2136, I also used my own script that included some of the more advanced route matching features: https://gist.github.com/skirtles-code/5fc5dc24f51d7132119365d0646c140f.
'Breaking changes'
I'm using the term 'breaking changes' very loosely here. I don't think any documented behaviour is broken, but there are some changes to undocumented behaviour that could (at least in theory) break people's applications. I don't think any of these cases is actually worth worrying about, but I also don't think it helps to keep them secret.
The 'breaking changes' only occur when calling
addRoute()
directly. If routes are configured via theroutes
option then everything should behave as before.Specifically, the difference occurs when adding a child route with
addRoute()
, if that child route has the same score as one of its ancestors. Here is a (somewhat convoluted and contrived) example that demonstrates the current behaviour:The example shows two routers,
router1
androuter2
.router1
specifies all the routes in the initial config.router2
only specifies the parent route, then adds the child later usingaddRoute()
. Notice how this leads to different outcomes. Withrouter1
, the child route is given priority, whereas withrouter2
, the parent route is given priority.The reason this happens is that parent/child relationships aren't explicitly considered. With
router1
, the route inchildren
is inserted first, then the parent, so the child gets priority. Withrouter2
, the parent gets added first, then the child, so the priority is reversed.The one common case where this arises is for
path: ''
on a child. There is special handling in the currentinsertMatcher()
to handle that specific case.With the changes I've proposed here, using
addRoute()
to add a child would change to behave the same way as thechildren
option. Sorouter2
would now yield the same ordering asrouter1
. I think this is more intuitive than the current behaviour. A consequence of this is that there's no longer any need to makepath: ''
a special case, we get that automatically.But while the new behaviour seems more consistent in the previous example, there are other cases where it could appear less consistent. For example:
In this example,
router2
androuter3
both behave the same way, resolving/a/b
to the routeother
. They differ fromrouter1
, but at least they're consistent with each other.With the new behaviour,
router3
would now be consistent withrouter1
, resolving/a/b
to the routechild
. Why? Because inrouter3
the parent route has the same score as its child, so the child is inserted before the parent, leapfrogging ahead of the routeother
.To reiterate, I'm not saying these examples show important, real-world use cases. But I think they help to better understand what has changed. Perhaps they'll help to identify other use case that actually are important.
Further work