Bug 97760 – CSSComputedStyleDeclaration::getPropertyCSSValue() triggering unnecessary relayouts and style recalcs

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

Currently getPropertyCSSValue() (which is also used to implement the more common getPropertyValue()) calls Document::updateLayoutIgnorePendingStylesheets() unconditionally. However only a few properties are actually layout dependent, making many of these relayouts unnecessary. Moreover, triggering full style recalc is also often unnecessary as the current node may already have valid style even if some other parts of the tree require recalc.

[Antti Koivisto]

Created attachment 165941 [details]
patch

[Antti Koivisto]

No performance test, the autocompleted title lied.

[Radar WebKit Bug Importer]

<rdar://problem/12384306>

[Gyuyoung Kim]

Comment on attachment 165941 [details]
patch

Attachment 165941 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13964087

[Build Bot]

Comment on attachment 165941 [details]
patch

Attachment 165941 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13967078

New failing tests:
http/tests/misc/acid3.html
fast/frames/seamless/seamless-css-cascade.html

[WebKit Review Bot]

Comment on attachment 165941 [details]
patch

Attachment 165941 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14003039

New failing tests:
fast/dom/shadow/shadow-nested-pseudo-id.html
fast/dom/shadow/link-in-shadow-tree.html
editing/shadow/contenteditable-propagation-at-shadow-boundary.html
fast/css/style-scoped/style-scoped-apply-author-styles.html
fast/frames/seamless/seamless-css-cascade.html

[WebKit Review Bot]

Comment on attachment 165941 [details]
patch

Attachment 165941 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14018911

New failing tests:
fast/dom/shadow/shadow-nested-pseudo-id.html
fast/dom/shadow/link-in-shadow-tree.html
editing/shadow/contenteditable-propagation-at-shadow-boundary.html
fast/css/style-scoped/style-scoped-apply-author-styles.html
fast/frames/seamless/seamless-css-cascade.html

[Antti Koivisto]

Created attachment 166099 [details]
updated patch

[WebKit Review Bot]

Comment on attachment 166099 [details]
updated patch

Attachment 166099 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14038873

New failing tests:
css3/filters/custom/custom-filter-property-computed-style.html

[Andreas Kling]

Comment on attachment 166099 [details]
updated patch

Kickass! r=me with the test cr-ews complains about fixed.

[WebKit Review Bot]

Comment on attachment 166099 [details]
updated patch

Attachment 166099 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14065029

New failing tests:
css3/filters/custom/custom-filter-property-computed-style.html

[Antti Koivisto]

http://trac.webkit.org/changeset/129844

[Eric Seidel (no email)]

Comment on attachment 166099 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166099&action=review

Sounds great. :)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389
> +static bool isLayoutDependentProperty(CSSPropertyID propertyID)

Slightly scary that this is a whitelist.  Might be better to list all properties to force those adding a property to make a choice as to if it depends on Layout or not.

[Antti Koivisto]

(In reply to comment #13)
> Slightly scary that this is a whitelist.  Might be better to list all properties to force those adding a property to make a choice as to if it depends on Layout or not.

I would agree in most cases but new layout dependent properties are very rare (and should generally be avoided). Also basic testing for newly added properties is likely reveal if this has been omitted. On balance I thought it was not worth having a giant switch here.

[Ojan Vafai]

Looks like this was a 40% improvement on the Chromium Win7 bot for dromaeo_jslibstylejquery!

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibstylejquery/report.html?history=100&rev=159617

[Simon Fraser (smfr)]

Antti, do you recall why CSSPropertyFilter is layout-dependent here?

[Antti Koivisto]

I don't. Reading the code I don't see any reason for it to be layout dependent either.

[Antti Koivisto]

Well, I see ews complained about css3/filters/custom/custom-filter-property-computed-style.html above so I suppose it was added to fix that. But it might be just full style recalc that it needed.