113098 - lazy calls to parentContext->GetStyleData

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

In bug 111815 I removed the last vestiges of having lazy calls to
parentContext->GetStyleData in the nsRuleNode::Compute*Data functions.  I think
we should make this lazy, and the easy way to do that would be to add two new
eRuleDetail values -- eRuleFullReset and eRulePartialReset.  This probably would
give some space savings.

Comment 1

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

Attachment: patch

Comment 2

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

I think we could use a little more |detail == eRuleNone| optimization too (for
the case where we don't have a start struct or do have a post-resolve callback.

Comment 3

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

Attachment: updated patch, plus extras

This also adds a little nsStyleContext cleanup and the renaming of the hated
mInheritBits to mDependentBits.

Comment 4

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

Attachment: a few more tweaks

Comment 5

[David Hyatt]

sr=hyatt.  

Comment 6

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

I should change
// XXXldb What about eRuleFullInherited?
to
// XXXldb What about eRuleFullInherited?  (Which is faster?)

Comment 7

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

For the record, here's a summary of what this patch does:

This patch restores an optimization that I removed in the patch to
bug 111815 by adding a few new values to the ruledetail.  The 
optimization is that we can avoid calling GetStyleData all the way up 
the style context tree in some cases (particularly for reset structs), 
and we may never need the data -- for example, we may only care about 
the TextReset struct for leaf and near-leaf frames.  This should be a
bit of a space savings and time savings.

It also renames the confusing nsRuleNode::mInheritBits to mDependentBits
(while leaving nsStyleContext::mBits, which is really (mostly) about
inheritance, unlike nsRuleNode::mDependentBits).

Finally, it uses none bits in a really minor case where they weren't 
used before, namely if there is one rule node that has a rule that
causes something to be specified, and then a rule node that is a
descendent from it that explicitly specifies 'inherit' for that property
so that there's again nothing specified and we can completely inherit.
For example, given:

div { line-height: 3; }
div.special { line-height: inherit; }

the rule nodes associated with |div.special| and their descendants could
have none bits set (and will if nothing else in the text struct is
specified).  This saves a needless struct copy (and the cost of computing
it) on all style contexts with one of the rule nodes that now gets none
bits.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Why are some rules using 

+  if (parentContext && 
+      aRuleDetail != eRuleFullReset &&
+      aRuleDetail != eRulePartialReset &&
+      aRuleDetail != eRuleNone)

to determine whether to get the parent style date while others use

+  if (parentContext && aRuleDetail != eRuleFullReset)

?  The first of this makes sense for the "textreset" and "uireset" rules but why
display, background, margin, border, padding, outline, and the like?  
Alternately, why is "visibility" using the second pattern?

Comment 9

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

The ones that use the former are the inherited structs -- the structs where the
properties are inherited by default ("Inherited: yes" in the spec).  The ones
that use the latter are the reset structs, where none of the properties are
inherited by default.

Comment 10

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

Except I got it backwards, but you get the point.  Too many != are confusing. :-)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 65380 [details] [diff] [review]
a few more tweaks

Ah.  Makes perfect sense.  :)

r=bzbarsky

Comment 12

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

Fix checked in, 2002-02-16 17:53 PST.

Comment 13

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

I added one additional comment change that I just caught (where "inherit" was
used to refer to the mInheritBits meaning of inherit):

-    return GetParentData(aSID); // We inherit. Just go up the rule tree and
return the first
-                                // cached struct we find.
+    return GetParentData(aSID); // We depend on an ancestor for this
+                                // struct since the cached struct it has
+                                // is also appropriate for this rule
+                                // node.  Just go up the rule tree and
+                                // return the first cached struct we
+                                // find.

Comment 14

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

For the record, this caused bug 131454.