Bug 81523 – Enable style sharing for elements with a style attribute

• Status: REOPENED
• Component: CSS

[Julien Chaffraix]

Currently this is explicitly disabled in the style sharing code. However this special casing is unneeded and could help us save some time / memory.

[Julien Chaffraix]

Created attachment 132606 [details]
Proposed change.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Julien Chaffraix]

Comment on attachment 132606 [details]
Proposed change.

/me needs to investigate the failures.

[Andreas Kling]

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.

[Julien Chaffraix]

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 :(

[Julien Chaffraix]

Created attachment 132869 [details]
Fixed bad copy 'n' paste, redid some benchmarkings.

[Andreas Kling]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 132947 [details]
Propsed refactoring 3: did not see any performance with this approach but it is cleaner.

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Julien Chaffraix]

Created attachment 133299 [details]
Simplified the logic per antti's discussion.

[Antti Koivisto]

Comment on attachment 133299 [details]
Simplified the logic per antti's discussion.

r=me

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

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.

[Julien Chaffraix]

(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).

[Julien Chaffraix]

(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.