1404140 - Consider flushing layout in a more fine-grained way in getComputedStyle.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

I happened to be reading some more Blink code today, and I think we could borrow another of their getComputedStyle optimizations.

In particular, we could borrow [1] without too much effort I think.

Instead of flushing layout from the beginning, we should consider flushing frames instead first, and then:

 * Only flush layout if the element has a primary frame.
 * Don't flush layout for properties if the property is fixed (in a per-property / per-frame basis). They don't use this for stuff like width / height which may be affected by other properties like max-width / max-height.

Not sure how worth it really is, I guess it depends on the content, but seems worth looking into.

[1]: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp?l=195&rcl=dc902f206d55e0d226fb7072ad383d1c2974073f

Comment 1

[Jet Villegas (inactive)]

SGTM!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Did ^ because I _think_ we're back to using [qf] again, but let me know if not.

Comment 3

[mattheww]

One situation where this would be valuable is when setting the dimensions of multiple elements using jQuery's .height() and .width().
Depending on the value of box-sizing, its setters can read padding and border-width, so doing this thrashes in Firefox and IE but not Safari or Chrome.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to mattheww from comment #3)

One situation where this would be valuable is when setting the dimensions of multiple elements using jQuery's .height() and .width().
Depending on the value of box-sizing, its setters can read padding and border-width, so doing this thrashes in Firefox and IE but not Safari or Chrome.

Just reading height thrashes layout though, doesn't it? (If the element is actually displayed, I mean)

Do you have a test-case I could profile? That'd be immensely useful to justify me working on this :)

Comment 5

[mattheww]

I worded that confusingly, sorry!
When you call $("#myElement").height(1234), jQuery will:

1. Read box-sizing (fine in all browsers)
2. Sometimes read padding-top/padding-bottom (depending on box-sizing)
3. Write height

I ran into this while rewriting some slow code that lined up two separate tables by copying row heights from one to the other. I uploaded a test-case doing the same thing to https://wartmanm.github.io/debugger-benchmarks/jquery-height.html.

I can imagine someone testing in only one browser, and stopping at copyHeights2, without realizing they hadn't solved the problem. But that didn't happen in this case, and I don't have any evidence that it's ever happened. Plus, I didn't spot it until now, but the documentation specifically warns that .height() can be slow: https://api.jquery.com/height/#entry-longdesc
So I don't know if this purely hypothetical scenario is worth it, really :(

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I think we should fix this. transform can have an even nicer optimization which Blink and WebKit don't have, which is that we don't need to update layout if the transform doesn't have any percentage...

Over to the queue.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1570721 - Add a content viewer API to emulate color scheme. r=bzbarsky

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Fix non-unified build bustage in nsComputedDOMStyle.cpp r=heycam

Depends on D40250

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Cleanup initialization of nsComputedDOMStyle::mFlushedPendingReflows. r=heycam

Depends on D40293

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Rename nsComputedDOMStyle::NeedsToFlush to NeedsToFlushStyle. r=heycam

Depends on D40294

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Factor out getting the outer and inner frame. r=heycam

Depends on D40295

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Return early when trying to flush to get the style of a disconnected element. r=heycam

We just don't return a styles for them anyway, and this simplifies the following
patches a bit.

Depends on D40296

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Refactor nsComputedDOMStyle::UpdateCurrentStyleSources so that it happens in two flushes, not one. r=heycam

We still don't optimize out the second based on the results of the first, but I
plan to do that in a bit. That's what this bug is about anyway.

Depends on D40297

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Don't flush layout for layout-dependent properties if we know that it won't affect the result. r=heycam

Depends on D40298

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404140 - Remove the GetCSNeedsLayoutFlush flag, as it is unneeded now. r=heycam

Depends on D40299

Comment 16

[Phabricator Automation]

Comment on attachment 9082390 [details]
Bug 1570721 - Add a content viewer API to emulate color scheme. r=bzbarsky

Revision D40250 was moved to bug 1570721. Setting attachment 9082390 [details] to obsolete.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86beb4dad0cf57887e72c7d680187941859cf763

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Err, wrong bug, but it also contains these patches so oh well.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c3c8a5e1c74
Fix non-unified build bustage in nsComputedDOMStyle.cpp r=heycam
https://hg.mozilla.org/integration/autoland/rev/6c76dadbf66b
Cleanup initialization of nsComputedDOMStyle::mFlushedPendingReflows. r=heycam
https://hg.mozilla.org/integration/autoland/rev/517527a3c682
Rename nsComputedDOMStyle::NeedsToFlush to NeedsToFlushStyle. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c4e1b1cdd8ce
Factor out getting the outer and inner frame. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e4797359e806
Return early when trying to flush to get the style of a disconnected element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/43ac67d814d4
Refactor nsComputedDOMStyle::UpdateCurrentStyleSources so that it happens in two flushes, not one. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b568a2a21f56
Don't flush layout for layout-dependent properties if we know that it won't affect the result. r=heycam
https://hg.mozilla.org/integration/autoland/rev/abe0d15df80f
Remove the GetCSNeedsLayoutFlush flag, as it is unneeded now. r=heycam

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dedea6ad59f9
followup: Avoid busting GCC base toolchain jobs which warn about a debug-only variable.

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2c3c8a5e1c74
https://hg.mozilla.org/mozilla-central/rev/6c76dadbf66b
https://hg.mozilla.org/mozilla-central/rev/517527a3c682
https://hg.mozilla.org/mozilla-central/rev/c4e1b1cdd8ce
https://hg.mozilla.org/mozilla-central/rev/e4797359e806
https://hg.mozilla.org/mozilla-central/rev/43ac67d814d4
https://hg.mozilla.org/mozilla-central/rev/b568a2a21f56
https://hg.mozilla.org/mozilla-central/rev/abe0d15df80f
https://hg.mozilla.org/mozilla-central/rev/dedea6ad59f9