[FIX]Speed up AddImportantRules

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

AddImportantRules pretty consistently shows up as about 20% of the time under FileRules; costs are QI, time spent in the function itself, the nsTArray ctor/dtor.

Patch coming up that makes the following changes:

1)  Moves GetImportantRule() to nsIStyleRule.
2)  Makes GetImportantRule() return a weak pointer.
3)  Keeps track of whether there are important rules in the rulewalker, and
    avoids calling AddImportantRules altogether if there aren't (and often
    there aren't).

This cuts down the time spent under AddImportantRules by a good bit (down to 3% of FileRules, in my measurements).
Created attachment 407056 [details] [diff] [review]
Fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #407056 - Flags: review?(dbaron)
Summary: Speed up AddImportantRules → [FIX]Speed up AddImportantRules
Comment on attachment 407056 [details] [diff] [review]
Fix

>diff --git a/layout/style/nsICSSStyleRule.h b/layout/style/nsICSSStyleRule.h
>--- a/layout/style/nsICSSStyleRule.h
>+++ b/layout/style/nsICSSStyleRule.h

We should really de-COM-taminate this at some point.

(And maybe we could move GetImportantRule back and make it non-virtual; I think it's relatively straightforward to never call it on non-CSS rules; we'd just have to drop some of the AssertNoImportantRules DEBUG code.  So, in other words, I'm not entirely happy about the change to move it to nsIStyleRule.)

> void
> nsStyleSet::AddImportantRules(nsRuleNode* aCurrLevelNode,
>                               nsRuleNode* aLastPrevLevelNode,
>                               nsRuleWalker* aRuleWalker)
> {
>-  if (!aCurrLevelNode)
>-    return;

This is needed; see what nsRuleWalker::Forward does on OOM.  (Maybe add a comment?)

You should also rev the IIDs of nsIStyleRule and nsICSSStyleRule.

r=dbaron with that.

Bug 522595 patch 6 will also speed up GetImportantRule a little.
Attachment #407056 - Flags: review?(dbaron) → review+
> And maybe we could move GetImportantRule back and make it non-virtual;

There are some UA-level rules that are not in fact nsICSSStyleRule (e.g. the restriction rules for first-letter and first-line).  We could work around that, I guess.  Would you prefer I just do that here, or as a followup?

> This is needed; see what nsRuleWalker::Forward does on OOM. 

Oh, hmm.  Yeah, indeed.  Will comment.

Though would it make more sense to have nsRuleNode::Transition return |this| on OOM?  It's not like the current setup actually works: nsStyleSet::ResolveStyleFor just passes ruleWalker.GetCurrentNode() without checking it for null to GetContext, which calls FindChildWithRules, which calls aRuleNode->IsRoot() and therefore crashes on null.

> You should also rev the IIDs of nsIStyleRule and nsICSSStyleRule.

Good catch; will do.
Followup is fine.

And maybe switching the OOM handling to return this would also make sense; perhaps another followup?

The deCOM should probably happen after all the patches-currently-floating-around land...
> And maybe switching the OOM handling to return this would also make sense;

Filed bug 529749.

> Followup is fine.

Filed bug 529750.
Blocks: 529749
Pushed http://hg.mozilla.org/mozilla-central/rev/b7824c1c8596
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.