Speedometer: Update to modern React version
Created attachment 318480 [details] Patch
Created attachment 318485 [details] Results before and after the patch
LGTM. Thanks for dropping the Yarn package lock in favor of an npm5 one too.
Hmm. Actually, hold up. Looking more closely at the data: there's a 10x difference in numbers. I'm seeing 25 at the median for STP before and 305 after. Do we know what might be impacting this at a library level? Might require input from the React team.
FWIW, the massive improvement is consistent across the tested browsers (Chrome, Safari, Firefox). I pinged Dan from the React team this morning (https://twitter.com/mathias/status/898450542732746752) but he’s on vacation and I didn’t really push for a response :)
Yeah, this doesn't look right.
Side note: the repository currently ignores all directories named `build`, which is a problem for this particular test — it depends on an asset in: resources/todomvc/architecture-examples/react/node_modules/director/build/ It would be easiest if these files could be checked in, so that the benchmark can be run after checking out the repo without additional `cd $dir && npm install` steps. Could the ignore rules be relaxed for `PerformanceTests/*`?
To make it easier to debug this further, I’ve put up both versions (before/after applying the patch) here: React v15.4.0: - App: https://speedometer-react-15-4-0.now.sh/resources/todomvc/architecture-examples/react/ - Benchmark: https://speedometer-react-15-4-0.now.sh/InteractiveRunner.html?suite=React-TodoMVC&startAutomatically ~560ms React v15.6.1: - App: https://speedometer-react-15-6-1.now.sh/resources/todomvc/architecture-examples/react/ - Benchmark: https://speedometer-react-15-6-1.now.sh/InteractiveRunner.html?suite=React-TodoMVC&startAutomatically ~25ms
(In reply to Mathias Bynens from comment #8) > To make it easier to debug this further, I’ve put up both versions > (before/after applying the patch) here: > > React v15.4.0: > - App: > https://speedometer-react-15-4-0.now.sh/resources/todomvc/architecture- > examples/react/ > - Benchmark: > https://speedometer-react-15-4-0.now.sh/InteractiveRunner.html?suite=React- > TodoMVC&startAutomatically ~560ms > > React v15.6.1: > - App: > https://speedometer-react-15-6-1.now.sh/resources/todomvc/architecture- > examples/react/ > - Benchmark: > https://speedometer-react-15-6-1.now.sh/InteractiveRunner.html?suite=React- > TodoMVC&startAutomatically ~25ms It seems that modern version does not create list items. (you can check it by clicking Step).
(In reply to Yusuke Suzuki from comment #9) > (In reply to Mathias Bynens from comment #8) > > To make it easier to debug this further, I’ve put up both versions > > (before/after applying the patch) here: > > > > React v15.4.0: > > - App: > > https://speedometer-react-15-4-0.now.sh/resources/todomvc/architecture- > > examples/react/ > > - Benchmark: > > https://speedometer-react-15-4-0.now.sh/InteractiveRunner.html?suite=React- > > TodoMVC&startAutomatically ~560ms > > > > React v15.6.1: > > - App: > > https://speedometer-react-15-6-1.now.sh/resources/todomvc/architecture- > > examples/react/ > > - Benchmark: > > https://speedometer-react-15-6-1.now.sh/InteractiveRunner.html?suite=React- > > TodoMVC&startAutomatically ~25ms > > It seems that modern version does not create list items. (you can check it > by clicking Step). Ooh, good catch! Interestingly, doing it manually still works in the patched version, so I’m not sure what’s going wrong here. Sounds like something in the runner?
You're most likely not forcing the render update in tests.js. Because some library schedule async tasks to update DOM, we're manually invoking functions to trigger a manual update.
Comment on attachment 318480 [details] Patch We're missing director.min.js. Did you forget to add it?
The issue is this: https://github.com/facebook/react/commit/045f1a791c6e17253e9d927ffca70ae5d00b4fe5 (which was later cherry-picked and backported to React v15) intentionally prevented React’s `onChange` from triggering after programmatic value changes. As a result, our `handleChange` never gets called when using the runner, so the new TODO item’s internal value never gets updated through the `setState` call. It’s stuck at the default value, i.e. the empty string. Therefore, when Enter is programmatically pressed and `handleNewTodoKeyDown` is called, the `if (val)` check fails, and the TODO item never gets added to the list. I’ve asked if there’s a reasonable way to work around this behavior without patching our React copy: https://github.com/facebook/react/pull/5746#issuecomment-323947947
(In reply to Ryosuke Niwa from comment #12) > Comment on attachment 318480 [details] > Patch > > We're missing director.min.js. Did you forget to add it? No, but see comment #7.
Created attachment 318748 [details] Patch
(In reply to Mathias Bynens from comment #15) > Created attachment 318748 [details] > Patch The new patch updates React and react-dom to v15.5.4 (released in May 2017). While this is not the latest stable version (that would be v15.6.1), it’s still recent enough to be considered modern. Once a workaround is found for React v15.6+’s handling of programmatic event triggering we can update again.
Comment on attachment 318748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318748&action=review > PerformanceTests/ChangeLog:8 > + This updates React and react-dom to v15.5.4 (released in May 2017). Looks like this patch doesn't update anything in the benchmark? It's simply removing a bunch of files and only updating package.json?
Comment on attachment 318748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318748&action=review >> PerformanceTests/ChangeLog:8 >> + This updates React and react-dom to v15.5.4 (released in May 2017). > > Looks like this patch doesn't update anything in the benchmark? It's simply removing a bunch of files and only updating package.json? Ah, yes that’s correct. v15.5.4 turns out to be the version Speedometer currently uses. This patch makes that explicit by pinning that version in `package.json`, documents the build steps, and gets rid of some unneeded files. > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/react/index.html:17 > + <script src="node_modules/director/build/director.min.js"></script> Directories named `build` seem to be ignored in this repository. Can we relax that restriction so that we can commit this file (without having to move it around)? Alternatively, we could add `mv node_modules/director/{build/,}director.min.js` to the build script (although that feels a bit hacky).
Created attachment 318855 [details] Patch
Created attachment 318856 [details] Patch
There doesn't appears to be a measurable difference in score for this subtest (each number is the average of 10 iterations). == Before == Safari 130.4 145.6 145.1 144.7 145.4 Chrome 106.6 107.2 104.1 102.2 100.8 Firefox 72.66 75.57 74.86 75.92 77.48 == After == Safari 145.2 144.7 145.6 145.2 147.6 Chrome 108.4 106.1 105.5 105.3 103.9 Firefox 71.99 76.40 76.39 73.30 73.59
Comment on attachment 318856 [details] Patch Clearing flags on attachment: 318856 Committed r221104: <http://trac.webkit.org/changeset/221104>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34044402>