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

RESOLVED FIXED in mozilla10

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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?
(Assignee)

Updated

7 years ago
Blocks: 608648
(Assignee)

Updated

7 years ago
Assignee: nobody → bzbarsky
Priority: -- → P2
I *think* this sounds ok, since it's all a within-struct dependency rather than a cross-struct dependency.
(Assignee)

Comment 2

6 years ago
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.
Attachment #541295 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Whiteboard: [need review]

Comment 3

6 years ago
Review ping
(Assignee)

Comment 4

6 years ago
Please don't do that.  If I decide this is a priority over dbaron's other work, I'll ping him myself, thanks.
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
Attachment #541295 - Flags: review?(dbaron) → review+
(Assignee)

Comment 6

6 years ago
> 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.
(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.
(Assignee)

Comment 8

6 years ago
> 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.
ok
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/6094246d2abc
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/6094246d2abc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.