Bug 175715 - Speedometer: Update to modern React version
Summary: Speedometer: Update to modern React version
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 172339
  Show dependency treegraph
 
Reported: 2017-08-17 23:59 PDT by Mathias Bynens
Modified: 2017-08-23 14:38 PDT (History)
9 users (show)

See Also:


Attachments
Patch (659.61 KB, patch)
2017-08-18 00:04 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Results before and after the patch (93.63 KB, application/pdf)
2017-08-18 00:38 PDT, Mathias Bynens
no flags Details
Patch (280.30 KB, patch)
2017-08-22 02:00 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Patch (290.77 KB, patch)
2017-08-23 00:50 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Patch (291.08 KB, patch)
2017-08-23 00:55 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Bynens 2017-08-17 23:59:11 PDT
Speedometer: Update to modern React version
Comment 1 Mathias Bynens 2017-08-18 00:04:52 PDT
Created attachment 318480 [details]
Patch
Comment 2 Mathias Bynens 2017-08-18 00:38:35 PDT
Created attachment 318485 [details]
Results before and after the patch
Comment 3 Addy Osmani 2017-08-18 07:43:33 PDT
LGTM. Thanks for dropping the Yarn package lock in favor of an npm5 one too.
Comment 4 Addy Osmani 2017-08-18 08:36:24 PDT
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.
Comment 5 Mathias Bynens 2017-08-18 11:48:07 PDT
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 :)
Comment 6 Ryosuke Niwa 2017-08-18 21:17:06 PDT
Yeah, this doesn't look right.
Comment 7 Mathias Bynens 2017-08-21 02:33:42 PDT
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/*`?
Comment 9 Yusuke Suzuki 2017-08-21 07:49:59 PDT
(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).
Comment 10 Mathias Bynens 2017-08-21 07:55:06 PDT
(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?
Comment 11 Ryosuke Niwa 2017-08-21 14:54:52 PDT
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 12 Ryosuke Niwa 2017-08-22 00:03:22 PDT
Comment on attachment 318480 [details]
Patch

We're missing director.min.js. Did you forget to add it?
Comment 13 Mathias Bynens 2017-08-22 01:01:48 PDT
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
Comment 14 Mathias Bynens 2017-08-22 01:02:18 PDT
(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.
Comment 15 Mathias Bynens 2017-08-22 02:00:37 PDT
Created attachment 318748 [details]
Patch
Comment 16 Mathias Bynens 2017-08-22 02:02:40 PDT
(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 17 Ryosuke Niwa 2017-08-22 23:07:43 PDT
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 18 Mathias Bynens 2017-08-23 00:02:40 PDT
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).
Comment 19 Mathias Bynens 2017-08-23 00:50:13 PDT
Created attachment 318855 [details]
Patch
Comment 20 Mathias Bynens 2017-08-23 00:55:10 PDT
Created attachment 318856 [details]
Patch
Comment 21 Ryosuke Niwa 2017-08-23 12:56:13 PDT
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 22 WebKit Commit Bot 2017-08-23 14:37:50 PDT
Comment on attachment 318856 [details]
Patch

Clearing flags on attachment: 318856

Committed r221104: <http://trac.webkit.org/changeset/221104>
Comment 23 WebKit Commit Bot 2017-08-23 14:37:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2017-08-23 14:38:33 PDT
<rdar://problem/34044402>