Bug 101163 – Exploit shared attribute data to avoid parsing identical "style" attributes.

• Component: CSS
• Status: RESOLVED FIXED

[Andreas Kling]

Build on top of bug 100990 to avoid parsing the "style" attribute more than necessary.

[Andreas Kling]

Created attachment 172242 [details]
Patch

[Build Bot]

Comment on attachment 172242 [details]
Patch

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

New failing tests:
fast/table/border-collapsing/rtl-border-collapsing-vertical.html
fast/table/border-collapsing/equal-precedence-resolution-vertical.html
fast/writing-mode/block-level-images.html

[WebKit Review Bot]

Comment on attachment 172242 [details]
Patch

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

New failing tests:
fast/table/border-collapsing/rtl-border-collapsing-vertical.html
fast/table/border-collapsing/equal-precedence-resolution-vertical.html
fast/writing-mode/block-level-images.html

[Andreas Kling]

Created attachment 174781 [details]
Patch II: The Hackening

[Antti Koivisto]

Comment on attachment 174781 [details]
Patch II: The Hackening

r=me

[Antti Koivisto]

Comment on attachment 174781 [details]
Patch II: The Hackening

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

> Source/WebCore/ChangeLog:18
> +        * css/StyleResolver.cpp:
> +        (WebCore::isCacheableInMatchedPropertiesCache):
> +
> +            Disable the matched properties cache for styles with non-standard writing-mode.
> +            This is necessary because some CSS properties have different meaning depending on context -
> +            properties handled by CSSProperty::resolveDirectionAwareProperty().
> +

Would be good to explain how this breaks with the other changes.

[Andreas Kling]

Created attachment 174782 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174782 [details]
Patch for landing

Clearing flags on attachment: 174782

Committed r135021: <http://trac.webkit.org/changeset/135021>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Hard to be sure it was this patch, but it seems likely that this was a ~1% improvement on one of the chromium page load tests: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl1/report.html?rev=168390&graph=times&trace=t&history=40

[Ojan Vafai]

Not sure how, but I think this patch may have regressed DOM/CreateNodes by ~9%: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dom_perf/report.html?rev=168452&graph=CreateNodes&trace=score&history=50

The regression range is http://trac.webkit.org/log/?verbose=on&rev=135028&stop_rev=135020. This seems like the only possible culprit in that range.

I believe that test suite is running http://trac.webkit.org/browser/trunk/PerformanceTests/DOM/CreateNodes.html.

I tried looking at the equivalent graphs at http://webkit-perf.appspot.com/, but I couldn't get it to load them.

[Andreas Kling]

(In reply to comment #11)
> Not sure how, but I think this patch may have regressed DOM/CreateNodes by ~9%: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dom_perf/report.html?rev=168452&graph=CreateNodes&trace=score&history=50
> 
> The regression range is http://trac.webkit.org/log/?verbose=on&rev=135028&stop_rev=135020. This seems like the only possible culprit in that range.
> 
> I believe that test suite is running http://trac.webkit.org/browser/trunk/PerformanceTests/DOM/CreateNodes.html.
> 
> I tried looking at the equivalent graphs at http://webkit-perf.appspot.com/, but I couldn't get it to load them.

Hrmf. I was afraid that might happen. I couldn't prove that it was slower on my machine, but the test is taking a slightly slower path when cloning the style attribute now.

We used to have an optimization to bypass the whole Element::attributeChanged() virtual dispatch for HTMLNames::styleAttr when cloning attributes and just call StyledElement::styleAttributeChanged() directly. 

While the main purpose of this was to dodge the reparsing of the inline CSS, it also avoided some things that I'm not entirely sure are OK to skip, notably DOM tree version revving and node lists invalidation. Not to mention that the code wasn't very future-proof since anything added to attributeChanged() in the future would also get skipped for style attributes.

I'm gonna see if I can make up for the regression somehow, because I'd like the cloned element to jump through all the same hoops as the source element. Filed bug 102598 for this.