Bug 76337 - Cache CSSStyleSelector::Features in RuleSets
Summary: Cache CSSStyleSelector::Features in RuleSets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-14 11:20 PST by Antti Koivisto
Modified: 2012-01-16 14:46 PST (History)
3 users (show)

See Also:


Attachments
patch (21.06 KB, patch)
2012-01-14 12:03 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated to tot (21.15 KB, patch)
2012-01-16 13:56 PST, Antti Koivisto
kling: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
build fix for bots (21.19 KB, patch)
2012-01-16 14:29 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2012-01-14 11:20:55 PST
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.
Comment 1 Antti Koivisto 2012-01-14 12:03:06 PST
Created attachment 122554 [details]
patch
Comment 2 Andreas Kling 2012-01-14 12:26:28 PST
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.
Comment 3 Antti Koivisto 2012-01-16 06:07:04 PST
(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.
Comment 4 Antti Koivisto 2012-01-16 13:56:10 PST
Created attachment 122681 [details]
updated to tot
Comment 5 Andreas Kling 2012-01-16 14:09:28 PST
Comment on attachment 122681 [details]
updated to tot

r=muppet
Comment 6 Early Warning System Bot 2012-01-16 14:13:15 PST
Comment on attachment 122681 [details]
updated to tot

Attachment 122681 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11254319
Comment 7 Antti Koivisto 2012-01-16 14:29:44 PST
Created attachment 122685 [details]
build fix for bots
Comment 8 Antti Koivisto 2012-01-16 14:46:21 PST
http://trac.webkit.org/changeset/105093