Bug 175715 – Speedometer: Update to modern React version

• Component: New Bugs
• Status: RESOLVED FIXED

[Mathias Bynens]

Speedometer: Update to modern React version

[Mathias Bynens]

Created attachment 318480 [details]
Patch

[Mathias Bynens]

Created attachment 318485 [details]
Results before and after the patch

[Addy Osmani]

LGTM. Thanks for dropping the Yarn package lock in favor of an npm5 one too.

[Addy Osmani]

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.

[Mathias Bynens]

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 :)

[Ryosuke Niwa]

Yeah, this doesn't look right.

[Mathias Bynens]

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/*`?

[Mathias Bynens]

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

[Yusuke Suzuki]

(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).

[Mathias Bynens]

(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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 318480 [details]
Patch

We're missing director.min.js. Did you forget to add it?

[Mathias Bynens]

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

[Mathias Bynens]

(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.

[Mathias Bynens]

Created attachment 318748 [details]
Patch

[Mathias Bynens]

(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.

[Ryosuke Niwa]

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?

[Mathias Bynens]

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).

[Mathias Bynens]

Created attachment 318855 [details]
Patch

[Mathias Bynens]

Created attachment 318856 [details]
Patch

[Ryosuke Niwa]

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

[WebKit Commit Bot]

Comment on attachment 318856 [details]
Patch

Clearing flags on attachment: 318856

Committed r221104: <http://trac.webkit.org/changeset/221104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34044402>