Bug 81523

Summary: Enable style sharing for elements with a style attribute
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: CSSAssignee: Julien Chaffraix <jchaffraix>
Status: REOPENED ---    
Severity: Normal CC: dglazkov, hyatt, kling, koivisto, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 82060    
Bug Blocks:    
Attachments:
Description Flags
Proposed change.
none
Archive of layout-test-results from ec2-cr-linux-04
none
Fixed bad copy 'n' paste, redid some benchmarkings.
none
Propsed refactoring 3: did not see any performance with this approach but it is cleaner.
none
Simplified the logic per antti's discussion. none

Description Julien Chaffraix 2012-03-19 10:19:42 PDT
Currently this is explicitly disabled in the style sharing code. However this special casing is unneeded and could help us save some time / memory.
Comment 1 Julien Chaffraix 2012-03-19 10:46:48 PDT
Created attachment 132606 [details]
Proposed change.
Comment 2 WebKit Review Bot 2012-03-19 11:04:41 PDT
Comment on attachment 132606 [details]
Proposed change.

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

New failing tests:
compositing/checkerboard.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
css1/text_properties/vertical_align.html
compositing/iframes/iframe-in-composited-layer.html
compositing/layer-creation/fixed-position-out-of-view.html
css3/unicode-bidi-isolate-basic.html
compositing/direct-image-compositing.html
compositing/geometry/empty-embed-rects.html
http/tests/inspector/inspect-element.html
compositing/repaint/overflow-into-content.html
css1/box_properties/float_elements_in_series.html
accessibility/aria-disabled.html
css1/units/color_units.html
css1/box_properties/clear_float.html
compositing/overflow/ancestor-overflow.html
css3/bdi-element.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/repaint/content-into-overflow.html
css1/text_properties/text_decoration.html
compositing/iframes/iframe-content-flipping.html
css3/zoom-coords.xhtml
compositing/overflow/repaint-after-losing-scrollbars.html
compositing/layer-creation/rotate3d-overlap.html
compositing/overflow/overflow-scroll.html
compositing/iframes/composited-iframe-scroll.html
http/tests/inspector/compiler-source-mapping.html
compositing/overflow/clip-descendents.html
accessibility/dimensions-include-descendants.html
css1/formatting_model/floating_elements.html
css2.1/20110323/abspos-containing-block-initial-001.htm
Comment 3 WebKit Review Bot 2012-03-19 11:04:45 PDT
Created attachment 132609 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Julien Chaffraix 2012-03-19 11:08:37 PDT
Comment on attachment 132606 [details]
Proposed change.

/me needs to investigate the failures.
Comment 5 Andreas Kling 2012-03-19 11:16:18 PDT
Off the cuff it looks like you never compare the style attributes, just check that they are both present (or not present) on both elements.
Comment 6 Julien Chaffraix 2012-03-20 11:02:12 PDT
Comment on attachment 132606 [details]
Proposed change.

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

Re-testing the good patch to see if the results are still matching.

> Source/WebCore/css/CSSStyleSelector.cpp:1385
> +    if (!!element->getAttribute(styleAttr) != !!m_styledElement->getAttribute(styleAttr))
> +        return false;
> +

Spot on Andreas, this should be:

if (element->inlineStyle() && element->getAttribute(styleAttr) != m_styledElement->getAttribute(styleAttr))
   return false;

Bad last minute copy and paste here :(
Comment 7 Julien Chaffraix 2012-03-20 12:03:02 PDT
Created attachment 132869 [details]
Fixed bad copy 'n' paste, redid some benchmarkings.
Comment 8 Andreas Kling 2012-03-20 12:11:46 PDT
Comment on attachment 132869 [details]
Fixed bad copy 'n' paste, redid some benchmarkings.

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

> Source/WebCore/css/CSSStyleSelector.cpp:1382
> +    if (element->inlineStyle() && element->getAttribute(styleAttr) != m_styledElement->getAttribute(styleAttr))
> +        return false;

This means we'll end up serializing the elements' inline styles every time we get here. (StyledElement::updateStyleAttribute)
It might be more efficient to simply compare the StylePropertySets for strict equality.
Comment 9 Julien Chaffraix 2012-03-20 16:36:56 PDT
Comment on attachment 132869 [details]
Fixed bad copy 'n' paste, redid some benchmarkings.

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

>> Source/WebCore/css/CSSStyleSelector.cpp:1382
>> +        return false;
> 
> This means we'll end up serializing the elements' inline styles every time we get here. (StyledElement::updateStyleAttribute)
> It might be more efficient to simply compare the StylePropertySets for strict equality.

Yes, I haven't measured a definitive performance hit due to the serialization though. I will try that and see if it makes sense.
Comment 10 Julien Chaffraix 2012-03-20 19:13:22 PDT
Created attachment 132947 [details]
Propsed refactoring 3: did not see any performance with this approach but it is cleaner.
Comment 11 Antti Koivisto 2012-03-22 10:07:39 PDT
Comment on attachment 132947 [details]
Propsed refactoring 3: did not see any performance with this approach but it is cleaner.

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

> Source/WebCore/css/CSSStyleSelector.cpp:1253
> +enum StyleComparison {
> +    AttributeStyle,
> +    InlineStyle
> +};
> +
> +template<StyleComparison> static bool propertiesEqual(const CSSProperty&, const CSSProperty&);

I don't understand the need for these templates. It seems to me that regular functions would work equally well and be less confusing.
Comment 12 Antti Koivisto 2012-03-22 10:15:08 PDT
Comment on attachment 132947 [details]
Propsed refactoring 3: did not see any performance with this approach but it is cleaner.

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

> Source/WebCore/css/CSSStyleSelector.cpp:1261
> +    if (aProperty.isImportant() != bProperty.isImportant()
> +        || aProperty.isInherited() != bProperty.isInherited() || aProperty.isImplicit() != bProperty.isImplicit())
> +        return false;

I think you should do these comparisons in all cases. Then you can eliminate the confusing templates. Cost of unnecessary boolean comparisons in attribute style case is likely to be meaningless.
Comment 13 Julien Chaffraix 2012-03-22 11:19:58 PDT
Created attachment 133299 [details]
Simplified the logic per antti's discussion.
Comment 14 Antti Koivisto 2012-03-22 12:14:55 PDT
Comment on attachment 133299 [details]
Simplified the logic per antti's discussion.

r=me
Comment 15 WebKit Review Bot 2012-03-22 13:36:51 PDT
Comment on attachment 133299 [details]
Simplified the logic per antti's discussion.

Clearing flags on attachment: 133299

Committed r111751: <http://trac.webkit.org/changeset/111751>
Comment 16 WebKit Review Bot 2012-03-22 13:36:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Julien Chaffraix 2012-03-23 09:46:32 PDT
It looks like DHTML page cycler regressed on linux following this change: http://build.chromium.org/f/chromium/perf/linux-release/dhtml/report.html?history=150&rev=128488

Reopening to investigate why this happened.
Comment 18 Julien Chaffraix 2012-04-03 12:53:44 PDT
(In reply to comment #17)
> It looks like DHTML page cycler regressed on linux following this change: http://build.chromium.org/f/chromium/perf/linux-release/dhtml/report.html?history=150&rev=128488
> 
> Reopening to investigate why this happened.

DHTML pages are more or less all based on using tons inline style. That explains why the benchmark was impacted so much by this change: it represents the worst case scenario (a lot of different inline styles).
Comment 19 Julien Chaffraix 2012-04-03 17:33:12 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > It looks like DHTML page cycler regressed on linux following this change: http://build.chromium.org/f/chromium/perf/linux-release/dhtml/report.html?history=150&rev=128488
> > 
> > Reopening to investigate why this happened.
> 
> DHTML pages are more or less all based on using tons inline style. That explains why the benchmark was impacted so much by this change: it represents the worst case scenario (a lot of different inline styles).

Yep, the issue is that we don't bail out in locateSharedStyle now. This makes the whole style sharing logic hotter but doesn't increase the sharing on those pages. This change helps our sharing in some context but it seems to add a lot of overhead in the context of the DHTML page cycler. I feel like solving bug 81865 would help.