Speed up pattern matching

[taion]

Fixes #3215

[taion]

cc @KyleAMathews

I have no clue why we support newlines in paths anyway. Are they even legal characters in URLs? I don't think they are. @mjackson?

[KyleAMathews]

Ooo nice! That dropped the matching time for the same route that took ~1.2 seconds down to ~0.2 seconds.

Even faster would be nice given a 100 page site still isn't that big but this is a huge start.

[taion]

Shoot, this PR is actually wrong.

[taion]

Fixed.

#1166 is no longer needed because we no longer decode URIs or URI components before feeding them into the route matching.

@KyleAMathews Can you run your timing against this updated branch as well now?

[KyleAMathews]

I tested the latest version and it takes roughly the same amount of time.

[taion]

What do you get for the timing against 0.13.x?

[KyleAMathews]

I timed things on bricolage.io and got something like 50ms. It's a bit hard to know how apples-to-apples that test is as a) it's against production compiled code vs. dev and b) I don't know if the same route there is last in line like it is currently. But presumably it is.

[KyleAMathews]

And actually with this PR, the execution time is somewhere between 70 and 120ms (most later runs towards lower end). When I said 200ms earlier, I was including time running some mixin code which runs after the routing is complete.

[taion]

The first run ought to take longer since pattern compilation is lazy.

I'm going to leave this up for a bit to get a little more review – found a few other simplifications here. It's not a rewrite, just a quick cleanup of what we have right now.

IMO the performance point here (where path-to-regexp is still like 10x faster per #3215) is another point in favor of splitting out our path matching into a separate third-party library rather than maintaining our own hand-rolled pattern matching DSL.

[timdorr]

Reviewers, removing whitespace makes this easier to read through: https://github.com/reactjs/react-router/pull/3217/files?w=1

[timdorr]

This doesn't regress? Why were we testing for ?'s and #'s before?

[taion]

This is all legacy crap. History deals with all of this now; you're just not going to get unescaped ?s and #s in the path any more.

[mjackson]

👍

[timdorr]

I'm curious where the meat of the speedup is. Is it removing the \s\S classes, or the fact that it's not greedily going to the end anchor?

[timdorr]

BTW, this LGTM 👍

[taion]

It's exactly this change that speeds up @KyleAMathews's use case: 6e908b5#diff-3740a979a31a412b32b93d9300983835R101

The rest is just taking this opportunity to clean stuff up, since I'm touching this code anyway.

[timdorr]

Well, that line is gone in the final diff. I think it's mainly that we don't double-add a (.*?) on there now, unless I'm missing something. That combined with the end of line anchor would seem to be a pretty painfully slow regex, I think.

I'm wondering if we should add some tests for _compilePattern to make sure the regex's being built are coming out as expected.

[taion]

The 2nd commit is just to clean up the code – the speed improvement comes entirely from the first commit. I guess it was just the ([\s\S]*?) at the end that was slow. Replacing it with a (.*?) is enough to get the full speedup, but really it's unnecessary.

That we have any special-casing there at all is because of our concept of non-greedy splats, which IMO should just be replaced entirely with greedy splats.

I don't think we should assert on the output of _compilePattern per se – I think it makes more sense to assert on the result of route matching, because it's closer to what we actually want.

[mjackson]

Nice work, @taion. Thanks! :D

[taion]

Parts of that code have been bothering me for a while now. I'm happy to have finally had the excuse to get to the root of it. :D

[EvHaus]

There's a small regression in this PR which affected me when upgrading from 2.0 to 2.1. In 2.0 <Link to="test" /> would work, but in 2.1 it must now be <Link to="/test" /> with the leading slash.

EDIT: Seems to be tracked here: #3279

[timdorr]

@EvNaverniouk You always had to use the leading slash. We don't yet support relative URLs.

[taion]

I'm just about done here, but that's actually because #3158 was incorrectly released as part of 2.1.0, even though it should not have been.

[jmurzy]

I'm getting a bunch of these on the client side, whereas things seem to be working fine with SSR.

Warning: [react-router] Location "page" did not match any routes

[timdorr]

Are you running 2.1.1 yet? We pushed that to revert #3158.

[jmurzy]

2.1.1 fixed it. Thanks!