Closed Bug 1404140 Opened 3 years ago Closed 1 year ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox57 --- wontfix
firefox70 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Regressed 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p3])

Attachments

(8 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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
Whiteboard: [qf]
SGTM!
Priority: -- → P3
Whiteboard: [qf]
Whiteboard: [qf]
Did ^ because I _think_ we're back to using [qf] again, but let me know if not.
Whiteboard: [qf] → [qf:f64][qf:p3]
Whiteboard: [qf:f64][qf:p3] → [qf:p3:f64]
Whiteboard: [qf:p3:f64] → [qf:p3]

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.

(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 :)

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 :(

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.

Flags: needinfo?(emilio)
Blocks: 1570759

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

Depends on D40296

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 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.

Attachment #9082390 - Attachment is obsolete: true
Assignee: nobody → emilio
Flags: needinfo?(emilio)

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

Pushed by emilio@crisal.io:
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
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/dedea6ad59f9
followup: Avoid busting GCC base toolchain jobs which warn about a debug-only variable.
Regressions: 1575546
Duplicate of this bug: 1474813
Regressions: 1585882
Regressions: 1590837
You need to log in before you can comment on or make changes to this bug.