Fixes #525 (cursor bug) by moving notify from setState callback to componentDidUpdate. #557
Conversation
This also eliminates react15CompatibilityMode stopgap setting. See: #525 (comment) for discussion.
|
What does the flame chart look like? When it's inside an event handler, and when it's outside an event handler (e.g. network callback). |
|
Not sure yet, but what I learned is |
|
Just ran some perf tests (react-redux-perf and redux-todomvc projects) on this change vs RC1. This change's perf is almost identical or sliiiightly better with one small tweak I just made; 10-15 ms faster total (out of 350ms) when clicking 10 times on the todo list with 10k items... basically negligible. |
jimbolla
changed the title
|
And compared to the pre-RC1 version (ie, the "with-bug" approach)? |
|
The test above was with compatibility=false, so that would be the "with-bug" approach. Setting it to true for my 2 test cases would let it run slightly faster in this case because none of my connected components would be affected by the stale props bug. |
|
Can we test this with deeper nested apps? Generally “cascading setState” is slow in React but maybe this isn’t an issue here? I don’t understand the change well enough to say. |
|
What would be a good project to use to test that? |
|
I don't have any suggestions :-( |
|
This LGTM as-is. Sounds like (assuming I'm reading these comments right) the perf is equivalent to the 5.0 betas/alphas. We can go live with this and clean things up in 5.1 when React 16 is available (and knock out sCU!) I suppose a further improvement would be a heuristic to determine if we even need to set the cDU, but I'm not even sure how you would go about doing that. Any chance you ran this against React master/16? I'm curious to see how that version would perform with this change. |
|
I have not tested against react master. Yesterday, I started contriving a more nested demo app and ended up shaving a ton of yaks. |
|
Can someone summarize for me what the current state of play is for 5.0? What pieces are and are not turned on?
|
|
Currently, the state of
I would like to run a perf test against React |
|
So, net win-win, then - perf up, annoying flags down, bug fixed? Sweet! |
|
Ran the todomvc test against react master. The difference between rc1 and latest was 314.2ms vs 322.7ms. So 8.5ms difference for 10 x 10,001 connected components. The margin is close enough that the same test probably produces slightly different results depending on the browser running it. It'll be interesting to see if there's any noticeable change once all the Fiber stuff is fully implemented in React. |
|
I know @ryanflorence was building a bunch of stuff to prepare for it in Router 4.0, but scrapped it because it's hard to design for a moving target. And I'd rather not contribute to any anxiety on the React team by releasing libraries that are trying to commit to stuff that's not yet finalized and creating breakage where there shouldn't be any. I'm guessing we'll get a way to enable Fiber in 16 and then can start playing with it and preparing for it. But no need to go crazy just yet :) |
…k to componentDidUpdate. (reduxjs#557) * Fixes reduxjs#525 (cursor bug) by moving notify from setState callback to cDU. This also eliminates react15CompatibilityMode stopgap setting. See: reduxjs#525 (comment) for discussion. * Optimizes perf of previous commit.
Instead of using the
setStatecallback, doingnotifyNestedSubsincomponentDidUpdateseems to avoid the cursor bug (#525) without the need for the compatibility setting BS. (I swear the first time I tried this, it didn't work; I must've screwed something up. /shruggie) I'll perf test this tonight when I'm at my home PC with all my test projects. Assuming that goes well, this is a much cleaner solution. Thanks @istarkov.