523148 - [FIX]Speed up AddImportantRules

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> And maybe switching the OOM handling to return this would also make sense;

Filed bug 529749.

> Followup is fine.

Filed bug 529750.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b7824c1c8596