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)

defect

Tracking

()

RESOLVED WORKSFORME
Performance Impact ?
Tracking Status
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: emilio, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: perf)

Attachments

(1 file)

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 :(.
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
Priority: -- → P3
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...
(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)
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)
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
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.
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).
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)
Whiteboard: [qf:?]
Keywords: perf

Hey emilio, would you mind retesting this? (Is it still an issue?)

Flags: needinfo?(emilio)

Nah, this wfm now, but maybe just because they changed the site.

Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(emilio)
Resolution: --- → WORKSFORME
Performance Impact: --- → ?
Whiteboard: [qf:?]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: