Bug 97760 - CSSComputedStyleDeclaration::getPropertyCSSValue() triggering unnecessary relayouts and style recalcs
Summary: CSSComputedStyleDeclaration::getPropertyCSSValue() triggering unnecessary rel...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-09-26 23:59 PDT by Antti Koivisto
Modified: 2017-05-27 00:12 PDT (History)
10 users (show)

See Also:


Attachments
patch (4.80 KB, patch)
2012-09-27 00:08 PDT, Antti Koivisto
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
updated patch (7.39 KB, patch)
2012-09-27 17:06 PDT, Antti Koivisto
kling: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2012-09-26 23:59:09 PDT
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.
Comment 1 Antti Koivisto 2012-09-27 00:08:52 PDT
Created attachment 165941 [details]
patch
Comment 2 Antti Koivisto 2012-09-27 00:09:49 PDT
No performance test, the autocompleted title lied.
Comment 3 Radar WebKit Bug Importer 2012-09-27 00:12:58 PDT
<rdar://problem/12384306>
Comment 4 Gyuyoung Kim 2012-09-27 00:17:04 PDT
Comment on attachment 165941 [details]
patch

Attachment 165941 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13964087
Comment 5 Build Bot 2012-09-27 02:18:37 PDT
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
Comment 6 WebKit Review Bot 2012-09-27 03:22:59 PDT
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
Comment 7 WebKit Review Bot 2012-09-27 04:24:34 PDT
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
Comment 8 Antti Koivisto 2012-09-27 17:06:51 PDT
Created attachment 166099 [details]
updated patch
Comment 9 WebKit Review Bot 2012-09-27 18:26:54 PDT
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
Comment 10 Andreas Kling 2012-09-27 19:05:00 PDT
Comment on attachment 166099 [details]
updated patch

Kickass! r=me with the test cr-ews complains about fixed.
Comment 11 WebKit Review Bot 2012-09-27 19:22:23 PDT
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
Comment 12 Antti Koivisto 2012-09-27 20:51:58 PDT
http://trac.webkit.org/changeset/129844
Comment 13 Eric Seidel (no email) 2012-09-27 23:12:00 PDT
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.
Comment 14 Antti Koivisto 2012-09-28 20:45:43 PDT
(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.
Comment 15 Ojan Vafai 2012-10-01 23:29:58 PDT
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
Comment 16 Simon Fraser (smfr) 2017-05-26 20:47:17 PDT
Antti, do you recall why CSSPropertyFilter is layout-dependent here?
Comment 17 Antti Koivisto 2017-05-27 00:09:55 PDT
I don't. Reading the code I don't see any reason for it to be layout dependent either.
Comment 18 Antti Koivisto 2017-05-27 00:12:35 PDT
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.