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.
Created attachment 78811 [details] patch This patch allows ~25-40% of elements on regular web sites share style as opposed to ~0% before.
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.
Comment on attachment 78811 [details] patch r-, found some test failures
<rdar://problem/8860136>
Antti: once you have a working patch, can you post memory and pageload performance data?
Created attachment 78990 [details] fix regressions
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.
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?
> > 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
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.
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.
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.
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.
http://trac.webkit.org/changeset/76012
http://trac.webkit.org/changeset/76012 might have broken GTK Linux 32-bit Debug