1481783 - React updates can trigger lots of GC activity

Status: NEW

(Core :: JavaScript: GC, defect, P5)

Description

[Jon Coppeard (:jonco)]

The React framework has a 'keyed element' mode which can end up removing and re-creating a lot of DOM nodes on every interface update.  This can end up triggering a lot of GC activity, as in the following profile:

https://perf-html.io/public/bb47a21ad037b21f19da0f770ae87d5105f40672/calltree/?hiddenThreads=3&invertCallstack&thread=5&threadOrder=0-2-3-4-5-1&v=3

There are a couple of strange things here:
 - lots of minor GCs triggered for full slots buffer, which tenure ~90% of the nursery
 - incremental GCs with a long gap after running the first few slices

There's a lot more information in this document about the investigation: https://docs.google.com/document/d/17sCEntVKQNbbz_8iOwkVm-UTIu9KeOkrojA0UVCiJxg/edit#

Comment 1

[Paul Bone [:pbone]]

See also: 1480477 for making some DOM operations here faster.

Here are some profiles

keyed: https://perf-html.io/public/bb47a21ad037b21f19da0f770ae87d5105f40672/marker-chart/?hiddenThreads=3&thread=5&threadOrder=0-2-3-4-5-1&v=3
indicies as keys: https://perf-html.io/public/7951959729612f8e6df83a5dd7b83380a7903fd9/marker-chart/?hiddenThreads=2&thread=4&threadOrder=0-2-3-4-1&v=3
noop reconciler: https://perf-html.io/public/73473cd8a68aa9427b0ca127416c3faae4cd8102/marker-chart/?hiddenThreads=3&thread=4&threadOrder=0-2-3-4-1&v=3
Vanilla JS: https://perf-html.io/public/99c2d71d834e0c6b810b091b37a97496586f2cf4/marker-chart/?hiddenThreads=3&thread=4&threadOrder=0-2-3-4-1&v=3

I guess Vanilla JS means not React.

Comment 2

[Paul Bone [:pbone]]

Run the benchmarks

Step 1: get the benchmarks and run the server
$ git clone [e-mail]:victorporof/js-framework-benchmark.git
$ cd js-framework-benchmark
$ git checkout perf-explorations
$ yarn install && yarn start
Step 2: in a separate terminal session: get, build, and link the noop reconciler
$ git clone [e-mail]:victorporof/noop-react-reconciler.git 
$ cd noop-react-reconciler
$ yarn install && yarn build && yarn link
Step 3: in a yet another separate terminal session: build and open a benchmark
$ cd js-framework-benchmark/frameworks/react-v16.1.0-keyed
$ yarn install && yarn build-prod
$ open http://127.0.0.1:8080/frameworks/react-v16.1.0-keyed
Optional: Switch to a noop reconciler
$ cd js-framework-benchmark/frameworks/react-v16.1.0-keyed
$ yarn link "noop-react-reconciler"
$ edit src/main.es6.js

Replace
let ReactDOM = require('react-dom');
with 
let ReactDOM = require('noop-reconciler').default;
and then

$ yarn build-prod
$ open http://127.0.0.1:8080/frameworks/react-v16.1.0-keyed
Optional: Use the development version of a framework
$ cd js-framework-benchmark/frameworks/react-v16.1.0-keyed
$ edit webpack.config.js

Replace
'process.env.NODE_ENV': '"production"'
with 
'process.env.NODE_ENV': '"development"'
and then

$ yarn build-dev
$ open http://127.0.0.1:8080/frameworks/react-v16.1.0-keyed
Optional: running from the console
In a browser console, type `window._benchRunLots()` to run the simple benchmark with a single step, or `window._benchAll()` to run the complex benchmark with multiple steps.
Step 4: other benchmarks
Repeat step 3 for `vanillajs-keyed`, `react-v16.1.0-non-keyed`, or `vue-v2.5.16-keyed` instead of  `react-v16.1.0-keyed`.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

When is 'keyed element' mode used and when not? And why?

Comment 4

[Kannan Vijayan [:djvj]]

This needs to be profiled again, with 'responsiveness' de-selected in the profiler settings.  This setting causes the profiler to gather responsiveness information, which affects the idle timer.

Comment 5

[Kannan Vijayan [:djvj]]

Just talked this over in QF triage.  See comment 4.

Comment 6

[Paul Bone [:pbone]]

(In reply to Olli Pettay [:smaug] from comment #3)
> When is 'keyed element' mode used and when not? And why?

My understanding is that this means every element in React's copy of the DOM and maybe also in the sequence of updates to the DOP gets an ID.  the ID must not be a number.

One important consideration is that this tends to be the recommended way to use React, 1) keyed is recommended, 2) keyed with numbers generates warnings in the console and from eslint.  Developers are told this is faster even though it may be slower (at least in Firefox).

Victor's document explains this in a little more detail.

Action: Determine if this keyed/non-keyed difference also coccurs in Chrome/others, and how it is different.  If Chrome does better here then we know we can as well, if it doesn't then we don't know (maybe we still can).

Comment 7

[Paul Bone [:pbone]]

(In reply to Kannan Vijayan [:djvj] from comment #4)
> This needs to be profiled again, with 'responsiveness' de-selected in the
> profiler settings.  This setting causes the profiler to gather
> responsiveness information, which affects the idle timer.

I'm going to NI victor for this.  He may be in the best position to do this but Jon and I will probably want to reproduce this anyway.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Paul, could you perhaps profile this, if victor is away or busy.

Comment 9

[Victor Porof [:vporof][:vp]]

I'll get right to it, I somehow missed this in my bugmail.

Comment 10

[Victor Porof [:vporof][:vp]]

Re-profiled with responsiveness turned off:

keyed: https://perfht.ml/2LTKx0f
non-keyed: https://perfht.ml/2NJpU8T
vanilla js simulating the "keyed" approach: https://perfht.ml/2LT67Cb
vanilla js simulating the "non-keyed" approach: https://perfht.ml/2NIqJii

Notice the difference in ContentRemoved/Content*Inserted markers between the keyed and non-keyed versions.

@smaug: 
React's keyed elements mode is a way of helping out with the "VDOM diffing" phase in all VDOM-based frameworks by uniquely tagging component instances with strings, which short-circuits more expensive comparisons: when keys are different, corresponding DOM nodes are removed and new ones are created in their place; when keys are identical, DOM node mutation is enforced. Not tagging instances in this way, or opting against content-based key generation in favor of ordering-based is prominently discouraged regardless of actual use case, thorough verbose console warnings and default eslint rules; it is *extremely* unlikely to find real-world code which doesn't use keys, or index-based keying.

React and React-like frameworks optimize for small scale changes in long lists, such as children swapping, or minimal inserting or removal -- something that benchmarks don't showcase as often, but might very well be the more common use case in the real world. In those cases, uniqueness described through childrens' content performs vastly better, as expected. Therefore, this optimisation is extremely useful when inserting, removing, swapping or otherwise altering a few child nodes among many (e.g. in long lists).

Community maintained test-cases are sometimes written in an atypical way which maximizes measured performance. For example, all of the "big three"[1][2][3] benchmarks for React don't tag generated children using keys, thus guaranteeing mutation instead of creation of DOM nodes. This is discouraged by React through console warnings and official documentation. Getting similar performance is possible via index-based keying or uniqueness described through childrens' ordering instead of content, which is also highly discouraged with default linting rules, e.g. AirBnB's "react/no-array-index-key" [4]. 

Real world applications which use content-based key generation (by far the most likely scenario) seem to perform poorly in Firefox (see also bug 1480477), when the optimised-for operations aren't happening (e.g. the contents of a list are changed). They also seem to cause more than usual GC, which this bug is about.

[1] http://mathieuancelin.github.io/js-repaint-perfs
[2] https://github.com/krausest/js-framework-benchmark
[3] https://localvoid.github.io/uibench
[4] https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

vporof, did you create the profiles using a Nightly or local build which had reasonable rust optimizations (default optimization level is very slow)?

Comment 12

[Victor Porof [:vporof][:vp]]

@Olli: I've profiled using the latest Nightly downloaded from https://www.mozilla.org/en-US/firefox/channel/desktop/

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Victor Porof [:vporof][:vp] from comment #10) 
>They also seem to cause more than usual GC,
> which this bug is about.

Not sure that is particularly unexpected, when the library clearly is creating and not reusing tons of objects. But sure, something to optimize somehow.

Comment 14

[Paul Bone [:pbone]]

Regarding the GC, The keyd profiles in particular still show quite a lot of objects being allocated directly into the tenured heap (30-60%), and a high tenure rate from the nursery (80%!!). (We didn't have all this data in the profiler last time I looked.) So my queries about something being pretenured or just some allocations always going to the tenured heap are still valid.

Last time I looked at this bug it looked as if it was one of the promise objects that was skipping the nursery, but I'm still not sure.

So if it's barriers being triggered when tenured->nursery pointers are created/set then looking at the profile it might be updating HTML tables, or setting onclick handlers, those are the two situations that I see.

Because I don't know web programming, my next question is will React build up real dom structures that arn't linked into the current document, and then link them in once it knows it wants to keep them?, that could be a problem, but I don't think it does that, isn't that what vdom is for?

Or one of these things that tables or onclick sometimes points to is being pretenured (something we may be able to action).

Comment 15

[Paul Bone [:pbone]]

I can't run the benchmarks locally.

[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /frameworks/react-v16.1.0-keyed/" "Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0"
[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /css/currentStyle.css" "Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0"
[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /frameworks/react-v16.1.0-keyed/dist/main.js" "Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0"
[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /frameworks/react-v16.1.0-keyed/dist/main.js" Error (404): "Not found"
[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /css/bootstrap/dist/css/bootstrap.min.css" "Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0"
[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /css/main.css" "Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0"
[Thu Feb 14 2019 16:43:02 GMT+1100 (AEDT)] "GET /favicon.ico" "Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0"

Comment 16

[Victor Porof [:vporof][:vp]]

Everything works fine for me while following the instructions in this document: https://docs.google.com/document/d/17sCEntVKQNbbz_8iOwkVm-UTIu9KeOkrojA0UVCiJxg/edit#heading=h.21galjx4ds7a

Make sure you follow all steps (Step 1, Step 2, Step 3).

Comment 17

[Victor Porof [:vporof][:vp]]

Did you manage to run the benchmarks Paul? Anything more I can help with?

Comment 18

[Paul Bone [:pbone]]

oops, sorry this slipped. I'll look today or tomorrow (hopefully).