Bug 52370 – Style sharing optimization no longer works on major web sites

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

The code in CSSStyleSelector::locateSharedStyle() that tries to share style information between element has been defeated by widespread use of certain CSS selectors (:first-child pseudo class and similar). The current implementation disables the sharing optimization for the whole page if one of these constructs is seen in any style sheet used by the page.  

This is currently preventing style sharing on basically all major sites: apple.com, nytimes.com, facebook.com, news.bbc.co.uk, engadget.com... all fail to do any sharing.

[Antti Koivisto]

Created attachment 78811 [details]
patch

This patch allows ~25-40% of elements on regular web sites share style as opposed to ~0% before.

[WebKit Review Bot]

Attachment 78811 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:1009:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2977:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2978:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2979:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2980:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2980:  Missing spaces around |  [whitespace/operators] [3]
Source/WebCore/css/CSSStyleSelector.cpp:2981:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2982:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2983:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2984:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2985:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2986:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2987:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSStyleSelector.cpp:2988:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 14 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 78811 [details]
patch

r-, found some test failures

[Simon Fraser (smfr)]

<rdar://problem/8860136>

[Simon Fraser (smfr)]

Antti: once you have a working patch, can you post memory and pageload performance data?

[Antti Koivisto]

Created attachment 78990 [details]
fix regressions

[WebKit Review Bot]

Attachment 78990 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:1021:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 78990 [details]
fix regressions

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

> Source/WebCore/ChangeLog:25
> +            MatML default style sheet has sibling rules. Extract them and also

MathML

> Source/WebCore/ChangeLog:38
> +            Track sibling rules and ids used in the stylesheets to allow much more fine grained rejection of cases

fine-grained

> Source/WebCore/ChangeLog:51
> +            MatML default style sheet has sibling rules which don't get noticed by the normal document

MathML

> Source/WebCore/css/CSSStyleSelector.cpp:424
> +    void collectIdsAndSiblingRulesFromList(HashSet<AtomicStringImpl*>& ids, OwnPtr<CSSRuleSet>& siblingRules, CSSRuleDataList* rules);

Can CSSRuleDataList* be const CSSRuleDataList*? It's hard to see what the "out" params are otherwise. Maybe put 'rules' first?

> Source/WebCore/css/CSSStyleSelector.cpp:574
> +    // This is used in the style sharing code to check for rules that prevent sharing.

Not clear what "This" refers to.

> Source/WebCore/css/CSSStyleSelector.cpp:1127
> +            goto returnResult;

Ugh, goto. Can the code be factored with a new method or function to avoid this?

> Source/WebCore/css/CSSStyleSelector.cpp:3011
> +    return relation == CSSSelector::DirectAdjacent
> +        || relation == CSSSelector::IndirectAdjacent
> +        || type == CSSSelector::PseudoEmpty
> +        || type == CSSSelector::PseudoFirstChild
> +        || type == CSSSelector::PseudoLastChild
> +        || type == CSSSelector::PseudoOnlyChild
> +        || type == CSSSelector::PseudoOnlyOfType
> +        || type == CSSSelector::PseudoNthChild
> +        || type == CSSSelector::PseudoNthOfType
> +        || type == CSSSelector::PseudoNthLastChild
> +        || type == CSSSelector::PseudoNthLastOfType;
> +}

Style rules are to put || at the end of the previous line.

> Source/WebCore/css/CSSStyleSelector.cpp:3053
> +    AtomRuleMap::iterator end = m_idRules.end();
> +    for (AtomRuleMap::iterator it = m_idRules.begin(); it != end; ++it)
> +    end = m_classRules.end();
> +        collectIdsAndSiblingRulesFromList(ids, siblingRules, it->second);
> +    for (AtomRuleMap::iterator it = m_tagRules.begin(); it != end; ++it)
> +    for (AtomRuleMap::iterator it = m_pseudoRules.begin(); it != end; ++it)
> +    if (m_universalRules)
> +        collectIdsAndSiblingRulesFromList(ids, siblingRules, m_universalRules.get());
> +}
>  

Should be able to use const_iterators here. Can the whole method be const?

[Kenneth Rohde Christiansen]

> > Source/WebCore/css/CSSStyleSelector.cpp:3011
> > +    return relation == CSSSelector::DirectAdjacent
> > +        || relation == CSSSelector::IndirectAdjacent
> > +        || type == CSSSelector::PseudoEmpty
> > +        || type == CSSSelector::PseudoFirstChild
> > +        || type == CSSSelector::PseudoLastChild
> > +        || type == CSSSelector::PseudoOnlyChild
> > +        || type == CSSSelector::PseudoOnlyOfType
> > +        || type == CSSSelector::PseudoNthChild
> > +        || type == CSSSelector::PseudoNthOfType
> > +        || type == CSSSelector::PseudoNthLastChild
> > +        || type == CSSSelector::PseudoNthLastOfType;
> > +}
> 
> Style rules are to put || at the end of the previous line.

From the style guide:
"5. Boolean expressions at the same nesting level that span multiple lines should have their operators on the left side of the line instead of the right side."

It seems that the code is correct according to the style guide

[Antti Koivisto]

Created attachment 79067 [details]
style fixes based on simon's comments

...except for the || placements. Didn't fix the stylebot warning either as this is a big patch without any additional refactoring.

[WebKit Review Bot]

Attachment 79067 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.h:114:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSStyleSelector.cpp:1027:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Shark says this patch reduces time spent in style selection over loading news.bbc.co.uk by ~45%. This translates into overall CPU usage reduction of > 25% as the profile is dominated by the style selector.

It also reduces RenderStyle memory use by ~25% for gain of some 150kB on this page.

[Dave Hyatt]

Comment on attachment 79067 [details]
style fixes based on simon's comments

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

r=me

Great patch.

> Source/WebCore/css/CSSStyleSelector.cpp:574
> +    // Collect all ids and rules using silbling selectors (:first-child and similar)

Typo.  "silbling" should be "sibling"

> Source/WebCore/css/CSSStyleSelector.cpp:2998
> +static inline bool isSiblingSelector(CSSSelector* selector)

I would just make this a member function of CSSSelector.  I can see us using it elsewhere.

[Antti Koivisto]

http://trac.webkit.org/changeset/76012

[WebKit Review Bot]

http://trac.webkit.org/changeset/76012 might have broken GTK Linux 32-bit Debug