608756 - Consider caching display structs in the ruletree even when floating/positioned

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now float not none and position:absolute/fixed force the display struct to not be cached in the ruletree.  But it seems to me that we could cache it fine if:

1)  We always set mOriginalDisplay when calling EnsureBlockDisplay in
    ComputeDisplayData().
2)  We added (and set in ComputeDisplayData()) an mOriginalFloat
3)  We changed ComputeDisplayData() to set the display/float from the mOriginal
    values as needed when aStartStruct is non-null (which is the only case when
    things could go bad).

The cost would be a tiny bit more code, and on 32-bit an extra word in nsStyleDisplay (on 64-bit, the 14 PRUint8s we'd end up with would still fit in two words).

The benefit is more style sharing and less style recomputation.

Am I missing something, or would this work?

Comment 1

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

I *think* this sounds ok, since it's all a within-struct dependency rather than a cross-struct dependency.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

A few issues I'm not entirely decided on:

1)  Instead of setting mDisplay from mOriginalDisplay if aStartStruct, is it worth it to just do that in the copy constructor?  Or is that used in other cases? 

2)  If we keep the current setup, is the aStartStruct test even needed?

Note that I considered not always storing mOriginalDisplay/mOriginalFloats; then I would need more complicated logic in ComputedDisplayData to decide whether to set mDisplay/mFloats from them.  That can be done if desired, of course; then the various changes to keep them in sync aren't needed.

Comment 3

[tenzoKritz]

Review ping

Comment 4

[Boris Zbarsky [:bzbarsky]]

Please don't do that.  If I decide this is a priority over dbaron's other work, I'll ping him myself, thanks.

Comment 5

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

Comment on attachment 541295 [details] [diff] [review]
Proposed fix

>+  if (aStartStruct) {
>+    // We ended up with aStartStruct's values of mDisplay and mFloats, but
>+    // those may not be correct if our style data overrides its position
>+    // or float properties.  Reset to mOriginalDisplay and
>+    // mOriginalFloats; it if turns out we still need the display/floats
>+    // adjustments we'll do them below.
>+    display->mDisplay = display->mOriginalDisplay;
>+    display->mFloats = display->mOriginalFloats;
>+  }

The null-check here looks optional -- maybe better to omit.  (Either way, as far as I can tell, the goal would be performance.)

>     if (nsCSSPseudoElements::firstLetter == aContext->GetPseudo()) {
>       // a non-floating first-letter must be inline
>       // XXX this fix can go away once bug 103189 is fixed correctly
>-      display->mDisplay = NS_STYLE_DISPLAY_INLINE;
>+      display->mOriginalDisplay = display->mDisplay = NS_STYLE_DISPLAY_INLINE;

I can't think of any reason this would cause problems -- but I also don't see why it's needed.  I'd prefer not resetting mOriginalDisplay here.

>-      // We can't cache the data in the rule tree since if a more specific
>-      // rule has 'position: static' we'll end up with problems with the
>-      // 'display' and 'float' properties.
>-      canStoreInRuleTree = PR_FALSE;

Leave a comment here explaining why it's ok that we don't set |canStoreInRuleTree|?

>-      // We can't cache the data in the rule tree since if a more specific
>-      // rule has 'float: none' we'll end up with the wrong 'display'
>-      // property.
>-      canStoreInRuleTree = PR_FALSE;

Same here.

>diff --git a/layout/style/nsStyleContext.cpp b/layout/style/nsStyleContext.cpp
>--- a/layout/style/nsStyleContext.cpp
>+++ b/layout/style/nsStyleContext.cpp
>@@ -370,19 +370,21 @@ nsStyleContext::ApplyStyleFixups(nsPresC
>   // doesn't get confused by looking at the style data.
>   if (!mParent) {
>     if (disp->mDisplay != NS_STYLE_DISPLAY_NONE &&
>         disp->mDisplay != NS_STYLE_DISPLAY_BLOCK &&
>         disp->mDisplay != NS_STYLE_DISPLAY_TABLE) {
>       nsStyleDisplay *mutable_display = static_cast<nsStyleDisplay*>
>                                                    (GetUniqueStyleData(eStyleStruct_Display));
>       if (mutable_display->mDisplay == NS_STYLE_DISPLAY_INLINE_TABLE)
>-        mutable_display->mDisplay = NS_STYLE_DISPLAY_TABLE;
>+        mutable_display->mOriginalDisplay = mutable_display->mDisplay =
>+          NS_STYLE_DISPLAY_TABLE;
>       else
>-        mutable_display->mDisplay = NS_STYLE_DISPLAY_BLOCK;
>+        mutable_display->mOriginalDisplay = mutable_display->mDisplay =
>+          NS_STYLE_DISPLAY_BLOCK;
>     }
>   }

Comment here that mOriginalDisplay doesn't matter for its cascading purpose, and we probably want it to match the "hypothetical" display for its hypothetical box calculation purpose?


r=dbaron with that

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The null-check here looks optional

Indeed.  I can take it out, sure.  I agree that it's not obvious which way is faster.

> but I also don't see why it's needed.

To enforce the invariant documented in the header that mDisplay == mOriginalDisplay unless floated or positioned.

I suppose we could relax the invariant, but it's conceptually simpler to have it, I think.

> Leave a comment here explaining why it's ok that we don't set |canStoreInRuleTree|?
...
> Same here.

Will do.

> Comment here that mOriginalDisplay doesn't matter for its cascading purpose, and we
> probably want it to match the "hypothetical" display for its hypothetical box
> calculation purpose?

Will do.

Comment 7

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

(In reply to Boris Zbarsky (:bz) from comment #6)
> > The null-check here looks optional
> 
> Indeed.  I can take it out, sure.  I agree that it's not obvious which way
> is faster.
> 
> > but I also don't see why it's needed.
> 
> To enforce the invariant documented in the header that mDisplay ==
> mOriginalDisplay unless floated or positioned.
> 
> I suppose we could relax the invariant, but it's conceptually simpler to
> have it, I think.

I find it a little weird given that the resulting data could be a start struct for another struct of data.  Then again, I suppose it just means that :first-letter always has an mOriginalDisplay of either none or inline.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I find it a little weird given that the resulting data could be a start struct for
> another struct of data.

Can it?  We set canStoreInRuleTree = false in this codepath.

> Then again, I suppose it just means that :first-letter always has an mOriginalDisplay of > either none or inline.

For a non-floating first-letter, yes.  I did document why this code is setting mOriginalDisplay.

Comment 9

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

ok

Comment 10

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6094246d2abc

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6094246d2abc