1481310 - Slow painting in React DevTools slows down interactions

Status: RESOLVED WONTFIX

(Core :: Web Painting, defect, P3)

Description

[:Harald Kirschner :digitarald]

OSX on 15" MBP with WebRender on Nightly.

STR:
- Install React DevTools: https://addons.mozilla.org/en-US/firefox/addon/react-devtools/
- Open React app: https://demos.creative-tim.com/material-dashboard-react/#/table
- Open DevTools and switch to React app
- Press arrow right to expand through the tree

https://perfht.ml/2LXUSxk

100 to 300 ms canvas rendering each time a node is selected.

Comment 1

[:Harald Kirschner :digitarald]

Correction, first recording is without WebRender. Looks much better with WebRender: https://perfht.ml/2niwo38

Comment 2

[:Harald Kirschner :digitarald]

Windows 10 with WebRender: https://perfht.ml/2Mr2epk

Comment 3

[Matt Woodrow (:mattwoodrow)]

Interestingly, this all looks like normal content, not <canvas>.

The react devtools is creating a crazy amount of layers (enable layers.draw-border=true, see all the green lines with the tree expanded) and we're running out of textures in the pool.

Allocating lots of textures is slow, and we're sitting waiting for that :(

Not sure what the react devtools are doing to end up with so many layers, haven't figured out how to debug them yet.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Oh, looks like they're using will-change:opacity rather aggressively, and each time you expand it adds new ones! Fun. Maybe we can convince them not to?

WR probably does better here since it doesn't really do anything for will-change.

Comment 5

[Markus Stange [:mstange]]

Matt, any ideas of how we could deal with the large layer count in a better way here? I'm a bit surprised that we're using tiles here, are these rows not smaller than the tile size?

I found https://github.com/facebook/react-devtools/pull/618/commits/b43bf68f709a7537bbf519019b0df8d71f79f6ae which added a willChange: opacity rule, and it might be the one that we're running into here. My reading of this commit is that it wasn't based on a perf investigation but rather on intuition. We could submit a PR to them to remove the rule.

Comment 6

[Matt Woodrow (:mattwoodrow)]

I am surprised too!

The relevant display list bit looks like this:

 Opacity p=0x156fe6820 f=0x156d2c020(Block(div)(1)) key=37 bounds(1080,2298,60,21990) layerBounds(1080,1878,60,21990) visible(1080,2298,120,21990) building(1080,2298,120,21990) componentAlpha(0,0,0,0) clip() asr(<0x156de9030>) clipChain(0x1585b1f20 <0,2040,60900,8802> [root asr]) ref=0x1524f8020 agr=0x156de8f10 (will-change=opacity) (opacity 1) layer=0x15241c800
    CompositorHitTestInfo p=0x1585ad020 f=0x156d2c020(Block(div)(1)) key=27 bounds(0,0,0,0) layerBounds(0,-420,0,0) visible(1080,2298,120,21990) building(1080,2298,120,21990) componentAlpha(0,0,0,0) clip(0,420,71790,24786) asr(<0x156de9030>) clipChain(0x15852e420 <0,420,71790,24786> [0x156de9030], 0x1585b1f20 <0,2040,60900,8802> [root asr]) ref=0x1524f8020 agr=0x156de8f10 (will-change=opacity) (hitTestInfo 0x1) hitTestArea(x=1080, y=2298, w=120, h=21990)
    Border p=0x156f03820 f=0x156d2c020(Block(div)(1)) key=9 bounds(1080,2298,60,21990) layerBounds(1080,1878,60,21990) visible(1080,2298,120,21990) building(1080,2298,120,21990) componentAlpha(0,0,0,0) clip(0,420,71790,24786) asr(<0x156de9030>) clipChain(0x15852e420 <0,420,71790,24786> [0x156de9030], 0x1585b1f20 <0,2040,60900,8802> [root asr]) ref=0x1524f8020 agr=0x156de8f10 (will-change=opacity) layer=0x156db1800


We should only be creating an active layer for will-change:opacity if nsDisplayOpacity::NeedsActiveLayer returns true, and that should require 64x64 pixels.

The bounds of the item definitely isn't that big, though the check is for the frame's overflow area which isn't shown here. Not obvious how it could be so much bigger though.

I think your github link is correct, the latest version of that code uses a border now, which matches the display list.

Comment 7

[:Harald Kirschner :digitarald]

Pull request https://github.com/facebook/react-devtools/pull/1088

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Decide if we want an active layer for opacity after we've created the item and can use the bounds.

Comment 9

[Jamie Nicol [:jnicol]]

Comment on attachment 8999770 [details] [diff] [review]
Decide if we want an active layer for opacity after we've created the item and can use the bounds.

Review of attachment 8999770 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review - was on a much needed holiday!

The change to require both dimensions to be large enough makes sense. Is that alone enough for this? Or is the visual overflow rect actually significantly different to the bounds?

I'm not sure about adding the extra code if the visual overflow rect is good enough, but don't feel too strongly.

will-change should require the developer to sign a waiver saying they've tested it in multiple browsers and it actually improves performance!

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mattwoodrow, could you have a look please?

Comment 11

[Matt Woodrow (:mattwoodrow)]

It looks like this has been fixed by React, so I can't easily test to answer Jamie's question.

I think we can skip landing this for now.