Summary: | Exploit shared attribute data to avoid parsing identical "style" attributes. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||
Component: | CSS | Assignee: | 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
Andreas Kling
2012-11-04 01:26:59 PST
Created attachment 172242 [details]
Patch
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 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 Created attachment 174781 [details]
Patch II: The Hackening
Comment on attachment 174781 [details]
Patch II: The Hackening
r=me
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. Created attachment 174782 [details]
Patch for landing
Comment on attachment 174782 [details] Patch for landing Clearing flags on attachment: 174782 Committed r135021: <http://trac.webkit.org/changeset/135021> All reviewed patches have been landed. Closing bug. 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 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. (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. |