Bug 76337 – Cache CSSStyleSelector::Features in RuleSets

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

[Antti Koivisto]

Currently whenever the style selector is updated we go through all the applicable rules and collect the used features again. We should keep the features around as part of the RuleSets and update them incrementally. Collecting the features will then be just a matter of taking the union of all features used by the RuleSets.

[Antti Koivisto]

Created attachment 122554 [details]
patch

[Andreas Kling]

Comment on attachment 122554 [details]
patch

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

Dumping superficial comments since you have to reup anyway.

> Source/WebCore/ChangeLog:13
> +        This is 1-2% CPU time reduction (engadged, nytimes) due less time spent in feature collection.

Typo, engadget.

> Source/WebCore/ChangeLog:25
> +            Add a field far caching the features.

Typo, for.

> Source/WebCore/css/CSSStyleSelector.cpp:225
> +    void addRule(CSSStyleRule* , CSSSelector*);

Extra space after CSSStyleRule*.

> Source/WebCore/css/CSSStyleSelector.cpp:230
> +    

Extra whitespace.

> Source/WebCore/css/CSSStyleSelector.cpp:1950
> +    if (selector->m_match == CSSSelector::Id && !selector->value().isEmpty())

I suspect selector->value() can't be empty if selector->m_match == CSSSelector::Id.
No need to change that in this patch as you're just moving the code though.

> Source/WebCore/css/CSSStyleSelector.cpp:1980
> +                if (selector->isSiblingSelector())
> +                    foundSiblingSelector = true;

You could change this to:
if (!foundSiblingSelector && selector->isSiblingSelector())
To avoid wasting cycles after a first sibling selector is found.

> Source/WebCore/css/CSSStyleSelector.h:225
> +    struct RuleSelectorPair {
> +        RuleSelectorPair(CSSStyleRule* rule, CSSSelector* selector) : rule(rule), selector(selector) { }

Seems like we should either hide RuleSelectorPair() or make it initialize rule and selector as null pointers.

[Antti Koivisto]

(In reply to comment #2)
> > Source/WebCore/css/CSSStyleSelector.h:225
> > +    struct RuleSelectorPair {
> > +        RuleSelectorPair(CSSStyleRule* rule, CSSSelector* selector) : rule(rule), selector(selector) { }
> 
> Seems like we should either hide RuleSelectorPair() or make it initialize rule and selector as null pointers.

I think it is better to keep simple structs simple. The constructor is there for convenience only.

[Antti Koivisto]

Created attachment 122681 [details]
updated to tot

[Andreas Kling]

Comment on attachment 122681 [details]
updated to tot

r=muppet

[Early Warning System Bot]

Comment on attachment 122681 [details]
updated to tot

Attachment 122681 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11254319

[Antti Koivisto]

Created attachment 122685 [details]
build fix for bots

[Antti Koivisto]

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