Last Comment Bug 608756 - Consider caching display structs in the ruletree even when floating/positioned
: Consider caching display structs in the ruletree even when floating/positioned
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Mac OS X
: P2 normal with 1 vote (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 608648
  Show dependency treegraph
 
Reported: 2010-11-01 10:01 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2011-10-30 13:53 PDT (History)
8 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (12.96 KB, patch)
2011-06-22 23:16 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dbaron: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2010-11-01 10:01:47 PDT
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-7 (review requests must explain patch) 2011-05-02 13:28:07 PDT
I *think* this sounds ok, since it's all a within-struct dependency rather than a cross-struct dependency.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-22 23:16:20 PDT
Created attachment 541295 [details] [diff] [review]
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 2011-10-20 09:17:32 PDT
Review ping
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-20 09:30:03 PDT
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-7 (review requests must explain patch) 2011-10-28 15:39:52 PDT
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 [:bz] (Out June 25-July 6) 2011-10-28 18:36:25 PDT
> 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-7 (review requests must explain patch) 2011-10-28 18:59:04 PDT
(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 [:bz] (Out June 25-July 6) 2011-10-28 21:43:09 PDT
> 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-7 (review requests must explain patch) 2011-10-28 21:46:51 PDT
ok
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-29 00:07:27 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/6094246d2abc
Comment 11 Ed Morley [:emorley] 2011-10-30 10:37:03 PDT
https://hg.mozilla.org/mozilla-central/rev/6094246d2abc

Note You need to log in before you can comment on or make changes to this bug.