Stack size issues #2922
Comments
|
I'd rather keep route matching synchronous. Which specific version did you use as the baseline for your benchmarks? |
|
v1.0.3 to v2.0.0-rc5 Again, I wouldn't hold v1.0.3 up as the performance king – it's still the heaviest part of our server-side rendering AFAICT (edit – nope... it's not). |
|
Of all the code I thought I'd have to optimize, front end open source JS code would probably have taken last place... If I put something up on a branch, can you test against it? |
|
Yep, for sure – should have time to test it tomorrow. |
|
Try this branch: #2923 I've unwound the recursion in |
|
I'd just like to say this is actually really exciting to me that the project has moved beyond the point of "works" to "lets make it work faster". Thanks for this issue, obviously we want route matching to be as fast as possible while still allowing for code-splitting on routes. |
|
I don't think there has been any explicit performance effort made on react-router, outside of really obvious stuff. These kinds of deep dives into integrated performance testing are likely to yield some serious gains. |
|
@taion that branch certainly helps flatten things out (by about a factor of 2) – now about 40 functions deep before we get to Obviously I'd love it if this stack could be a little flatter, but I don't know enough about the internals to see if there's some more low-hanging fruit in there. In terms of performance, I think it's probably better to create some smaller benchmarks where the rest of React isn't going to get in the way – it's hard to isolate react-router as the sole cause of performance issues in our case – the deep stacks make it look like it may be responsible, but there's a good chance it might just be a red herring in our case. I'll play around some more and let you know if anything jumps out. |
|
Oh, darn, thought that'd kill all the stack bloat. It makes perfect sense to me that this sort of thing is not going to JIT well – unless V8 is ridiculously aggressive about inlining recursive function calls. Let me see if I can flatten this out a little bit more. How specifically are you running benchmarks? |
|
@taion just using |
|
What's your route tree look like? I'd love to get some performance tests in place so we can compare versions over time. Actually, that's one of my startup ideas: A CI service that also integrates performance data over time. |
|
Route tree is pretty flat and looks like: <Route path='/prefix' component={a}>
<IndexRoute component={b} />
<Route path='a.html' component={c} />
<Route path='b.html' component={d} />
<Route path='c.html' component={e} />
{/* 13 more exact matches */}
<Route path='d/:id.html' component={f} />
<Route path=':id.html' component={g} />
<Route path=':id1/:id2.html' component={h} />
<Route path=':id1/:id2/:id3.html' component={i} />
<Route path='*' component={NotFound} />
</Route>And I'm testing one of the Gonna create a simpler version of what we've got and test it out a bit – now that I can see the stack a little clearer, I'm less convinced that react-router's actually having a noticeable impact on performance. |
|
I unwound a bit more of the stack and rebased by branch. It's not going to clear as many stack frames, but it should clear a few. Can you when you get a chance and let me know how it looks? The code is far enough down into internals and well enough covered by tests that I'm not worried about complexity there. As long as it's not e.g. introducing a perf regression, I'd be inclined to merge it just so people get more usable stack frames. |
|
@taion great stuff, cut it in half again, much easier to read! |
|
Ok, so now that I can traverse the stacks a little easier, I can see that there are a bunch of other Will do a quick check to see if I can get some more accurate numbers on performance differences (if any) between the versions – it could be that @taion's changes have made a noticeable speed-up, although I suspect it's something else that's changed in our code that caused what I saw earlier. |
|
Changed this issue to be purely about the stack size – I think any performance exploration needs something a bit more scientific than what I can see in our app, so I'll open a separate issue if I build a reproducible benchmark that shows anything. @taion's changes in #2923 are a huge help in viewing stack traces and performance profiles though, so once that gets merged I say we close this issue. |
|
Based on the profiling you've done so far, how likely do you think it is that #2923 does not introduce performance regressions? |
|
Very likely – I see no noticeable difference in performance (either negative or positive) between #2923 and 2.0.0-rc5 |
|
I've always heard rumblings from the mythical v8 devs that the engine better handles iterative code, rather than recursive. I don't know the internals well enough to know if that's true. It may matter if the JITer is warmed up enough, along with other caches. So, my inclination is that the code changes in #2923 are probably marginally faster. |
|
At a glance the main JIT optimization possible with iterative code that would be difficult to apply in the recursive case would be inlining, which is probably going to be the biggest potential gain here anyway (avoid a bunch of extra jumps). |
Investigating a slowdown from 1.x to 2.x on our server-side rendering – we're seeing around a 15-25% hit going to 2.x (edit – this may be a red herring and not due to react-router)
I've run a profile, but it's quite hard to read because of the amount of recursion performed when matching routes – and this actually hasn't really changed from 1.x so I'm not sure that it's even contributing.
It would be great if plain 'ol synchronous routes didn't have this incredible amount of recursion – we only have 20 routes in our app, but route matching accounts for the highest amount of CPU used in our app – much more than the React rendering itself (edit – no it doesn't, the stacks just made it look that way).
To give you an illustration, here's a profile (taken with
node --prof) of our app – it's about 80 functions deep before you can even see therenderToStringfunction – this makes it incredibly hard to profile and track down performance issues:I really think the route matching needs to be converted to an iterative algorithm instead of a recursive one – if
getChildRoutesis encountered, then that'll need callback treatment, but normalchildRoutesshould just be able to be iterated over flatly.If that's not possible, then I wonder if it's worth considering making it truly asynchronous and processing each route on a different tick in the event loop – although I realise that this may actually make it less performant, though the function stacks would be a bit more sane.
Thoughts? Has anyone else encountered performance issues with this library?
The text was updated successfully, but these errors were encountered: