Bug 101163

Summary: Exploit shared attribute data to avoid parsing identical "style" attributes.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, koivisto, macpherson, menard, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Patch II: The Hackening
koivisto: review+
Patch for landing none

Description Andreas Kling 2012-11-04 01:26:59 PST
Build on top of bug 100990 to avoid parsing the "style" attribute more than necessary.
Comment 1 Andreas Kling 2012-11-04 01:36:19 PST
Created attachment 172242 [details]
Patch
Comment 2 Build Bot 2012-11-04 02:07:59 PST
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
Comment 3 WebKit Review Bot 2012-11-04 02:48:38 PST
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
Comment 4 Andreas Kling 2012-11-16 16:43:20 PST
Created attachment 174781 [details]
Patch II: The Hackening
Comment 5 Antti Koivisto 2012-11-16 16:49:57 PST
Comment on attachment 174781 [details]
Patch II: The Hackening

r=me
Comment 6 Antti Koivisto 2012-11-16 16:52:39 PST
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.
Comment 7 Andreas Kling 2012-11-16 16:55:48 PST
Created attachment 174782 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-11-16 17:35:14 PST
Comment on attachment 174782 [details]
Patch for landing

Clearing flags on attachment: 174782

Committed r135021: <http://trac.webkit.org/changeset/135021>
Comment 9 WebKit Review Bot 2012-11-16 17:35:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Ojan Vafai 2012-11-17 13:14:58 PST
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
Comment 11 Ojan Vafai 2012-11-17 14:18:13 PST
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.
Comment 12 Andreas Kling 2012-11-17 14:36:12 PST
(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.