Cameron McCormack (:heycam) Assignee
	Description • 9 years ago

In bug 931668 we made the restyling process stop as it traverses down the tree if we can be sure that the styles of descendant frames won't change from a given point.  This still can end up restyling the children of a frame that only has a non-inherited property change, to check whether their styles are the same.  For example with:

  <div>
    ... 1000 <p> elements ...
  </div>

if we set div.style.opacity = 0.5 then we'll still restyle all 1000 <p> children to check whether we can cut off the restyling after them.

If we track whether a given style context explicitly inherited a non-inherited property (or otherwise used it), we could stop the restyling process immediately after restyling the <div>.

See bug 1136893 for a proof of concept patch and some comments.

	Cameron McCormack (:heycam) Assignee
	Comment 1 • 9 years ago

Explicitly inheriting non-inherited properties is rare, so I think we can just have a single bit that tracks whether any non-inherited property was explicitly inherited, rather than trying to track individual properties.

	Cameron McCormack (:heycam) Assignee
	Comment 2 • 9 years ago

I was hoping this would help with bug 1172239 but I realise (of course) that the same conditions that make it safe to return eRestyle_Stop on children are the same conditions we'd need to check on those children from the parent, if returning this new restyle hint.  Because the children of the element we change the non-inherited property on in that page have shared style contexts, we can't stop restyling.

	Cameron McCormack (:heycam) Assignee
	Comment 3 • 9 years ago

I realised that the set of conditions is smaller when considering whether to stop at the element itself, so I have some patches that are mostly working now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e101f2e477 -- couple of test failures still.

	Cameron McCormack (:heycam) Assignee
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eac3b90c5c4

	Cameron McCormack (:heycam) Assignee
	Comment 12 • 9 years ago

One of the XXXs I left in Part 7 was about whether we need to worry about ::before/::after frame creation in MoveStyleContextsForChildren.  I'm pretty sure now we don't have to check for them, since we would have got an eRestyle_Subtree in that case.

(Leaving these reviewless until dbaron can accept reviews again.)

	Cameron McCormack (:heycam) Assignee
	Comment 15 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6545de5925b2

	David Baron :dbaron:
	Comment 16 • 9 years ago

Comment on attachment 8641012 [details] [diff] [review]
Part 1: Record on style contexts that reset style is explicitly inherited.

I think I reviewed a patch in another bug (maybe 1172087) that uses
0x400000000, so be careful when merging.

I think it's worth filing a followup on making setting this bit
more exact.  But if you're getting useful wins from just this
optimization, then this seems worth landing.

>+      /* This is pessimistic; we could be uncacheable because we relied on */ \
>+      /* the parent's font-size, for example, which does not need to */       \

Would that even fall into this case, or would it hit conditions.Cacheable() ?

r=dbaron

	David Baron :dbaron:
	Comment 17 • 9 years ago

Comment on attachment 8641013 [details] [diff] [review]
Part 2: Move style-if-visited as well in nsStyleContext::Move, if its parent is the same as the non-visited-style parent.

>+    aNewParent->AddRef();
>+    mStyleIfVisited->mParent->RemoveChild(mStyleIfVisited);
>+    mStyleIfVisited->mParent = aNewParent;
>+    mStyleIfVisited->mParent->AddChild(mStyleIfVisited);
>+    oldParent->Release();

You should probably either:
 (1) set oldParent = mStyleIfVisited->mParent before this chunk and
     use oldParent on the second of the quoted lines, or
 (2) use mStyleIfVisited->mParent instead of oldParent on the fifth
     of the quoted lines
(I know you assert they're the same, but...)

>Bug 1180120 - Part 2: Move style-if-visited as well in nsStyleContext::Move, if its parent is the same as the non-visited-style parent.

The commit message's use of "if" is a bit odd.  You're asserting
that that, not testing it.

I'm not sure I see where this is going, though.  Are you planning
to change ElementRestyler::ComputeRestyleResultFromNewContext so that
we execute this code?  (Or am I wrong in thinking the first early
return there prevents us from doing so?)

r=dbaron

	David Baron :dbaron:
	Comment 18 • 9 years ago

Comment on attachment 8641014 [details] [diff] [review]
Part 3: Record on ElementRestyler whether we are at the root of a restyle.

Maybe mRootOfRestyle instead of just mRoot?  (Or mIsRootOfRestyle?)

r=dbaron with that

	David Baron :dbaron:
	Comment 19 • 9 years ago

Comment on attachment 8641015 [details] [diff] [review]
Part 4: Add an outparam to CalcStyleDifference to indicate which struct pointers were identical.

>+  // We check for struct pointer equality here rather than as part of the
>+  // DO_STRUCT_DIFFERENCE calls, since those calls can result in structs
>+  // we previously examined and found to be null on this style context
>+  // getting computed by later DO_STRUCT_DIFFERENCE calls (which can
>+  // happen when the nsRuleNode::ComputeXXXData method looks up another
>+  // struct.)

Could you extend the comment to briefly (one sentence?) explain why you want
the after-state rather than the before-state?

r=dbaron

	David Baron :dbaron:
	Comment 20 • 9 years ago

Comment on attachment 8641018 [details] [diff] [review]
Part 6: Replace HasSameCachedStyleData call with samePointerStructs bit tests.

>+      // XXX This loop could be rewritten as bit operations on oldContext->mBits

Maybe FIXME rather than XXX?  (and then rewrap)

	David Baron :dbaron:
	Comment 21 • 9 years ago

Comment on attachment 8642222 [details] [diff] [review]
Part 6.2: Refactor MaybeReframeFor{Before,After}Pseudo.

I feel like these used to be more different in terms of the performance
characteristics of getting before and after frames being different; it
looks like Mats changed that in bug 907396 (changeset bf4849f9b835).  (I
worry that I may have made them more different again in bug 1110277
patch 3 (changeset 1bbebe9fec17).)  So I guess this is now a valid
refactoring.

The before and after cases previously different in whether they passed
mContent (::after) or aContent (::before) to AppendChange.  Your code
passes aContent, which matches ::before.  I can't figure out any cases
when mContent and aContent would be different, though.

r=dbaron

	David Baron :dbaron:
	Comment 22 • 9 years ago

Comment on attachment 8642223 [details] [diff] [review]
Part 7: Add eRestyleResult_StopWithStyleChange. (v2)

nsStyleStructFwd.h:

>+// Bits for inherited structs.
>+#define NS_STYLE_INHERITED_STRUCT_MASK \
>+  ((1 << nsStyleStructID_Inherited_Count) - 1)
>+// Bits for reset structs.
>+#define NS_STYLE_RESET_STRUCT_MASK \
>+  (((1 << nsStyleStructID_Reset_Count) - 1) \
>+   << nsStyleStructID_Inherited_Count)

This has a slightly hidden implicit dependency on nsStyleStructID being
32-bit (or, in particular, the same size as an integer literal 1).

Maybe cast the 1 to nsStyleStructID, if that compiles?  (Or maybe
declare an nsStyleStructID_size_t like nsRestyleHint_size_t, etc.)

>+  // XXX Bail out if we have out-of-flow or placeholder frames; not sure if it's
>+  //     safe to just skip them.

Do you need to do this?  And also the other XXX comments?

(Maybe this one actually isn't ready for review?  I thought you
didn't request it only because I wasn't accepting reviews.)

	Cameron McCormack (:heycam) Assignee
	Comment 23 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-4 (busy Aug. 8-Aug. 30) from comment #22)
> Comment on attachment 8642223 [details] [diff] [review]
> Part 7: Add eRestyleResult_StopWithStyleChange. (v2)
...
> >+  // XXX Bail out if we have out-of-flow or placeholder frames; not sure if it's
> >+  //     safe to just skip them.
> 
> Do you need to do this?  And also the other XXX comments?

The XXX applies to the "not sure if it's safe to just skip them", which I don't know the answer to still.

For the HasChildThatUsesGrandancestorStyle XXX, I hadn't looked into whether that check indeed was needed.  However this check will disappear once (if) we refcount style structs, which is why I didn't bother thinking too hard about it.

For the undisplayed content XXX, I just didn't bother with adding a method analagous to RestyleUndisplayedContent, as my main focus was getting something that worked for the Amazon page.  Still, we should handle this (now or soon).

	David Baron :dbaron:
	Comment 24 • 9 years ago

Comment on attachment 8642223 [details] [diff] [review]
Part 7: Add eRestyleResult_StopWithStyleChange. (v2)

>+      mRestyleTracker.AddRestyleRootsIfAwaitingRestyle(descendants);
>+      if (aRestyleHint & eRestyle_SomeDescendants) {
>+        AddPendingRestylesForDescendantsMatchingSelectors(mContent);
>+      }

I should perhaps come back to this bit another time, since I haven't
digested that patch (which I believe bz reviewed).  But for now I'll
trust you on this being what's needed, although you should be aware
that I didn't review this bit well.



Why does RestyleSelf not process struct swaps when it returns
StopWithStyleChange, such that if we can't Stop, we need to process them
later?  I don't understand that bit -- and it seems like it could at least
use a comment.




In ComputeRestyleResultFromFrame:

>+  // need to check antthing.

s/antthing/anything/

(and same in ComputeRestyleResultFromNewContext)

>   if (oldContext->HasChildThatUsesGrandancestorStyle()) {
>     LOG_RESTYLE_CONTINUE("the old context uses grandancestor style");
>-    return eRestyleResult_Continue;
>+    aRestyleResult = eRestyleResult_Continue;
>+    aCanStopWithStyleChange = false;
>+    return;
>   }

I wonder if aCanStopWithStyleChange is really needed here -- would
the bit that says a child inherits has a non-inherited be set?  This
probably doesn't need to be fixed now, but maybe worth a followup if
that's the case?  (If it is, would need to drop the early return.)


RestyleSelf:


>+    // If we have to restyle descendants, we can't return
>+    // eRestyleResult_StopWithStyleChange.
>+    if (aRestyleHint & (eRestyle_Subtree | eRestyle_LaterSiblings |
>+                        eRestyle_Force | eRestyle_ForceDescendants)) {

Comment should probably say "children" rather than "descendants" given
the inclusion of "Force", which only implies children and not other
descendants.

It's also not clear why LaterSiblings needs to be there.  Is it something
to do with struct swapping?


MoveStyleContextsForChildren:

>+  // XXX Bail out if there are undisplayed or display:contents children.
>+  //     We could get this to work if we need to.

This should probably be written as:
  // Bail out if there are undisplayed or display:contents children.
  // FIXME: We could get this to work if we need to.
since the "XXX" is currently confusing.

I think it's worth filing a followup bug at least for undisplayed children;
they're reasonably common.

>+  if (MustCheckUndisplayedContent(undisplayedParent)) {
>+    nsCSSFrameConstructor* fc = mPresContext->FrameConstructor();
>+    if (fc->GetAllUndisplayedContentIn(undisplayedParent) ||
>+        fc->GetAllDisplayContentsIn(undisplayedParent)) {

I don't see the MustCheckUndisplayedContent method in my tree -- not sure
what introduces it -- but is it correct that you're conditioning the
display:contents check on it?

MoveStyleContextsForContentChildren:

>+  // XXX Bail out if we have out-of-flow or placeholder frames; not sure if it's
>+  //     safe to just skip them.

Please split this in half and move it below to where you do it, with the
FIXME before the part that's to be fixed.

>+      if (child->GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
>+        return false;
>+      }

I think it should be safe to just skip out-of-flows.  Their style
contexts come from someplace else.  So I think this could just be a
continue.  (Although I'm ok fixing that in a followup if you're worried
about it.  But I think it should be fine.)

Skipping placeholders for now seems reasonable, though.  (That's where
their style contexts come from.)

Maybe file a followup bug on not skipping placeholders, though?


r=dbaron with that

Sorry about the delay getting to this.  We should probably try to get this uplifted to both aurora and beta.

	Cameron McCormack (:heycam) Assignee
	Comment 25 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy Aug. 8-Aug. 30) from comment #16)
> Comment on attachment 8641012 [details] [diff] [review]
> Part 1: Record on style contexts that reset style is explicitly inherited.
...
> >+      /* This is pessimistic; we could be uncacheable because we relied on */ \
> >+      /* the parent's font-size, for example, which does not need to */       \
> 
> Would that even fall into this case, or would it hit conditions.Cacheable() ?

If it's relying on the parent's font-size, then it would be uncacheable.  If it relies on the current element's font-size, then it's cacheable with dependencies.  Might be better to use a different example, though.

	Cameron McCormack (:heycam) Assignee
	Comment 26 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy Aug. 8-Aug. 30) from comment #17)
> Comment on attachment 8641013 [details] [diff] [review]
> Part 2: Move style-if-visited as well in nsStyleContext::Move, if its parent
> is the same as the non-visited-style parent.
...
> I'm not sure I see where this is going, though.  Are you planning
> to change ElementRestyler::ComputeRestyleResultFromNewContext so that
> we execute this code?  (Or am I wrong in thinking the first early
> return there prevents us from doing so?)

You would've eventually seen, but it gets invoked by ElementRestyler::MoveStyleContextsForChildren in part 7.

	Cameron McCormack (:heycam) Assignee
	Comment 27 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy Aug. 8-Aug. 30) from comment #24)
> Comment on attachment 8642223 [details] [diff] [review]
> Part 7: Add eRestyleResult_StopWithStyleChange. (v2)
...
> Why does RestyleSelf not process struct swaps when it returns
> StopWithStyleChange, such that if we can't Stop, we need to process them
> later?  I don't understand that bit -- and it seems like it could at least
> use a comment.

Are you asking why we can't we just do the struct swaps unconditionally when returning StopWithStyleChange?  It's not necessary to do the swap -- we have checked that the only difference in cached struct pointers is the reset structs that have changed.

> >   if (oldContext->HasChildThatUsesGrandancestorStyle()) {
> >     LOG_RESTYLE_CONTINUE("the old context uses grandancestor style");
> >-    return eRestyleResult_Continue;
> >+    aRestyleResult = eRestyleResult_Continue;
> >+    aCanStopWithStyleChange = false;
> >+    return;
> >   }
> 
> I wonder if aCanStopWithStyleChange is really needed here -- would
> the bit that says a child inherits has a non-inherited be set?  This
> probably doesn't need to be fixed now, but maybe worth a followup if
> that's the case?  (If it is, would need to drop the early return.)

I'd have to think a bit more deeply to see if this one can be dropped.  Once we do shared style structs we probably won't need to track grandancestor-style-usage anyway.

> RestyleSelf:
> 
> 
> >+    // If we have to restyle descendants, we can't return
> >+    // eRestyleResult_StopWithStyleChange.
> >+    if (aRestyleHint & (eRestyle_Subtree | eRestyle_LaterSiblings |
> >+                        eRestyle_Force | eRestyle_ForceDescendants)) {
> 
> Comment should probably say "children" rather than "descendants" given
> the inclusion of "Force", which only implies children and not other
> descendants.
> 
> It's also not clear why LaterSiblings needs to be there.  Is it something
> to do with struct swapping?

I think it can be removed.  Will verify.

> >+  if (MustCheckUndisplayedContent(undisplayedParent)) {
> >+    nsCSSFrameConstructor* fc = mPresContext->FrameConstructor();
> >+    if (fc->GetAllUndisplayedContentIn(undisplayedParent) ||
> >+        fc->GetAllDisplayContentsIn(undisplayedParent)) {
> 
> I don't see the MustCheckUndisplayedContent method in my tree -- not sure
> what introduces it -- but is it correct that you're conditioning the
> display:contents check on it?

Yes.

	Cameron McCormack (:heycam) Assignee
	Comment 28 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3eab8797d64

	Pulsebot
	Comment 29 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/88e5e1fc929a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dac8ab1e1a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8a95346a08
https://hg.mozilla.org/integration/mozilla-inbound/rev/6229c2f38ea9
https://hg.mozilla.org/integration/mozilla-inbound/rev/129945037746
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb20dbe84bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8942f7f4edd
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed938d9bb87
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b33fb7dc51

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/88e5e1fc929a
https://hg.mozilla.org/mozilla-central/rev/4dac8ab1e1a7
https://hg.mozilla.org/mozilla-central/rev/ed8a95346a08
https://hg.mozilla.org/mozilla-central/rev/6229c2f38ea9
https://hg.mozilla.org/mozilla-central/rev/129945037746
https://hg.mozilla.org/mozilla-central/rev/bcb20dbe84bd
https://hg.mozilla.org/mozilla-central/rev/f8942f7f4edd
https://hg.mozilla.org/mozilla-central/rev/1ed938d9bb87
https://hg.mozilla.org/mozilla-central/rev/25b33fb7dc51

	Cameron McCormack (:heycam) Assignee
	Comment 31 • 9 years ago

Comment on attachment 8641012 [details] [diff] [review]
Part 1: Record on style contexts that reset style is explicitly inherited.

(applies to all patches on the bug)

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: very poor performance on amazon.com (bug 1172239)
[Describe test coverage new/current, TreeHerder]: patches been on m-c for a week and a bit, manual testing confirmed it (along with other dependencies of bug 1172239) solved the performance issue
[Risks and why]: moderate; restyle code is complex, and risks include incorrectly styled content if the patches are buggy
[String/UUID change made/needed]: N/A

	Sylvestre Ledru [:Sylvestre]
	Comment 32 • 9 years ago

Comment on attachment 8641012 [details] [diff] [review]
Part 1: Record on style contexts that reset style is explicitly inherited.

We want to fix that for 42, let's take it.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1fbc7d972ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/f5eb4b908add
https://hg.mozilla.org/releases/mozilla-aurora/rev/07d4bad39bc4
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbf8d7d2b935
https://hg.mozilla.org/releases/mozilla-aurora/rev/7fca6840789d
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ddcf355f740
https://hg.mozilla.org/releases/mozilla-aurora/rev/80143eb3ab73
https://hg.mozilla.org/releases/mozilla-aurora/rev/121de382cd52
https://hg.mozilla.org/releases/mozilla-aurora/rev/124b3fa18283