Closed
Bug 1375032
Opened 7 years ago
Closed 6 years ago
Composing on the chromium-review tool at normal speed is impossible due to style and layout flushes that trigger frame construction and reflow.
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
WORKSFORME
Performance Impact | ? |
People
(Reporter: emilio, Unassigned)
References
(Blocks 1 open bug, )
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
text/plain
|
Details |
STR: Go to https://chromium-review.googlesource.com/c/517940/, hit the reply button, and compose a message.
Expected: I can type at a normal speed.
Actual results: I can type only a key or two each second.
Profile: http://bit.ly/2rReSaS
Seems like a bunch of time is spent on ProcessRestyledFrames doing frame construction, and another bunch of time reflowing the world.
It seems bug 1363805 may help (if it happens to be the case that whatever we're querying style isn't dirty at the time), but Firefox is quite unusable in that page :(.
Reporter | ||
Comment 1•7 years ago
|
||
I looked a bit more in-depth into it, and they're calling getBoundingClientRect a few times from the input key event handler. So the layout flush is really unavoidable then.
This likely means that we either reflow too much or too slow.
Blocks: FastReflows
Updated•7 years ago
|
Priority: -- → P3
Comment 2•7 years ago
|
||
Likely related to bug 1377253, and likely needs some flex layout optimizations. Looks like the textarea that you're typing into is inside of this element:
<gr-textarea id="textarea" ...>
That gr-textarea element and its 3 closest ancestors are all "display:flex". (Their sizing characteristics don't visibly change when you type a character, but if their parent decides they need a reflow, we'll do the same from-scratch process every time. I wonder why they're getting marked as dirty? The dirtiness from the new text should only propagate up to the actual <textarea> element...)
Also: the bug goes away if you resize your viewport to be less than 800px wide (at which point the UI changes slightly). I'm not sure if the speed difference is because of a CSS difference in their mobile UI, or if it's due to them swapping out the expensive event handler for a cheaper one...
Updated•7 years ago
|
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Likely related to bug 1377253, and likely needs some flex layout
> optimizations. Looks like the textarea that you're typing into is inside of
> this element:
> <gr-textarea id="textarea" ...>
>
> That gr-textarea element and its 3 closest ancestors are all "display:flex".
> (Their sizing characteristics don't visibly change when you type a
> character, but if their parent decides they need a reflow, we'll do the same
> from-scratch process every time. I wonder why they're getting marked as
> dirty? The dirtiness from the new text should only propagate up to the
> actual <textarea> element...)
I think in this case it's more likely to not be (particularly) related to flex reflow perf.
Most of the time seems reframing, and I suspect we should be able to avoid most if not all of it. Of course a lot of the rest of the time is just us reflowing the just-recreated frames, but I suspect avoiding the reframe in the first place should avoid most of the layout time too.
David, does that sound reasonable?
ni? myself too so I can take a closer look at why are we reframing that much and report back.
Flags: needinfo?(emilio)
Flags: needinfo?(dholbert)
Comment 4•7 years ago
|
||
Ah, right -- I did notice we're reframing first, and that makes sense as an explanation for why we think stuff's dirty and why we end up aggressively reflowing.
So: agreed that we should investigate the reframing & try to avoid that, as the first attempt at improving things here.
Flags: needinfo?(dholbert)
Reporter | ||
Comment 5•7 years ago
|
||
Well, so each time I press a key, we reframe the <main> _two times_... There's definitely some room for improvement there...
Here it is a single keypress, and the reconstructs it triggers (async = 1 don't really count, since they're just restyle events)
RecreateFramesForContent(0x7fffb5df99d0, async = 0): gr-overlay
nsCSSFrameConstructor::WipeContainingBlock: blockContent=0x7fffb80e7350
RecreateFramesForContent(0x7fffb80e7350, async = 1): main
RecreateFramesForContent(0x7fffb80e7350, async = 0): main
RecreateFramesForContent(0x7fffb5df99d0, async = 0): gr-overlay
RecreateFramesForContent(0x7fffb80e7350, async = 1): main
RecreateFramesForContent(0x7fffb80e7350, async = 0): main
Reporter | ||
Comment 6•7 years ago
|
||
There goes a single keypress in current central. Disclaimer: looks pretty bad :/
I'm trying to figure out what is each element / content that's getting removed and appended there.
Reporter | ||
Comment 7•7 years ago
|
||
So whenever I type we arrive to a change list that looks like:
=> 0x7f84fb222400 (#text), nsChangeHint_ReconstructFrame
=> 0x7f84fa4fd8f0 (br), nsChangeHint_ReconstructFrame
=> 0x7f84fa4fe8b0 (br), nsChangeHint_ReconstructFrame
=> 0x7f84fa4fe940 (br), nsChangeHint_ReconstructFrame
=> 0x7f84fb223e00 (#text), nsChangeHint_ReconstructFrame
Which looks pretty reasonable, and we handle quite well.
Then _afterwards_, some script in the page or whatever changes the <gr-overlay> element, in a way that we think it needs a reframe. This element is an inline in an IB split, so we just give up and reframe the whole <main>.
I have yet to discover why we think we need to reframe the <gr-overlay> element (which is the scroll frame holding the dialog).
Reporter | ||
Comment 8•7 years ago
|
||
So what they're doing is the following: Once I press the key, they remeasure the container, which has a style attribute like:
outline: medium none currentcolor; z-index: 103; position: fixed; top: 171px; left: 155px; box-sizing: border-box; max-height: 940px; max-width: 960px;
And ends up like:
outline: medium none currentcolor; z-index: 103;
After that, they flush, so the "position" change from "fixed" to the default value causes us to reconstruct (note that Blink doesn't reconstruct the world in that case, but it's pretty hard for us).
Reframing reframes the whole <main>, due to all the IB split situation explained above.
Later, they put back part of the styles removed, until it looks like:
outline: medium none currentcolor; z-index: 103; position: fixed; top: 0px; left: 0px; box-sizing: border-box;
Then flush, which is another <main> reframe.
Afterwards, they add the max-height:
outline: medium none currentcolor; z-index: 103; position: fixed; top: 0px; left: 0px; box-sizing: border-box; max-height: 940px;
And then flush, which is a reflow.
Then, finally, they add the max-width property, and flush, which is another reflow.
So there isn't anything particularly dumb we're doing. Chromium does better, I think because they don't reframe on position changes (they don't have the same placeholder frame setup as we do). But typing there also consumes large amounts of CPU in Chromium...
So I don't think there is anything particularly actionable or dumb to address here. Avoiding reframing on position changes would be nice, but it's not an easy task.
Clearing ni? for now, if Daniel or somebody else thinks anything related to the above is actionable in its current form (and not a researchy / long-term project, like getting rid of nsPlaceholderFrame), please speak :)
Flags: needinfo?(emilio)
Updated•7 years ago
|
Comment 9•6 years ago
|
||
Hey emilio, would you mind retesting this? (Is it still an issue?)
Flags: needinfo?(emilio)
Reporter | ||
Comment 10•6 years ago
|
||
Nah, this wfm now, but maybe just because they changed the site.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(emilio)
Resolution: --- → WORKSFORME
Updated•3 years ago
|
Performance Impact: --- → ?
Whiteboard: [qf:?]
You need to log in
before you can comment on or make changes to this bug.
Description
•