931668 - when possible, avoid creating new style contexts for all descendants of element whose style is changing

Status: RESOLVED FIXED

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

Description

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

One of the problems we've had with restyling performance has been
with the performance of changes to non-inherited properties on
elements with large numbers of descendants.  (It's not worse than
for inherited properties -- it's just that we could do better.)

A common case of this is dynamic changes to the CSS 'transform'
property, which can be handled very efficiently in the layer system,
and where, thus, restyling performance can be a significant
component of the performance.  This shows up, for example, on the
Firefox OS homescreen, where dynamic changes to CSS 'transform' are
used to track the user dragging between screens.  (This is the
performance problem during dragging; the problem with an extra
restyling performance cost at the start of a drag is separate, and
is related to the CSS used by the homescreen rather than being
something that should be fixed in Gecko.)  See bug 862276 for some
other thoughts on how to improve this case, though I now think this
approach is better.

The reason we spend a bunch of time is that we currently maintain
a number of invariants that require us to create new style contexts
for all the descendants of the element whose style is changing.
These invariants center on the idea that style contexts are
immutable, an invariant that's very useful to us for doing
comparisons between old and new style data and thus performing the
correct updates when style changes.  In particular, style contexts
have an immutable parent style context (representing the style they
inherit) and an immutable rule node (representing the sequence of
rules matched by the elements).  The performance cost of creating
the new style contexts is not trivial, but the main cost is actually
the cost of regenerating style structs that we don't keep around
between new and old contexts, either because they're for
non-inherited properties but we're unable to cache the struct in the
rule tree, or because they're for inherited properties that are
changed relatively leaf-ward in the tree (and thus changed many
times, requiring many recomputations).  Prior to Firefox OS 1.0.1 we
manually optimized some of the homescreen's CSS to reduce the cost
of this struct recomputation.

However, we should fix the bug in general, which is likely to help
Web content.  I think we should do this by, during the restyle
operation, detecting that the new style context (with a different
parent pointer but same rule node) will have no structs different
from the old style context, and when that happens, instead of
installing the new style context on the element, replace the parent
pointer of the old style context.

This will require some care with existing code that may depend on
current assumptions.  In particular:

 * we need to check that nsStyle*::CalcDifference methods always
   return non-zero for all changes.  (If there are any cases where
   that's currently a problem, we should probably fix them by giving
   nsChangeHint_BorderStyleNoneChange a more general name.)

 * we need to check that this approach is compatible with skipping
   of structs in nsStyleContext::CalcStyleDifference

 * we need to check that there aren't any ownership issues where
   child style contexts depend on memory owned by their parent
   (e.g., with calc())

 * we need to check there aren't any style computation issues where
   grandchild style contexts look at their grandparents during
   computation.  If there are, we'd probably need to set bits to
   indicate that.

 * probably some other things

Comment 1

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #0)
>  * we need to check that nsStyle*::CalcDifference methods always
>    return non-zero for all changes.  (If there are any cases where
>    that's currently a problem, we should probably fix them by giving
>    nsChangeHint_BorderStyleNoneChange a more general name.)

These are the cases where it looks like we need to start returning a change hint:

* -moz-system-font changes on nsStyleFont
* mBorder on nsStyleBorder (~specified value)
* combinations of outline-width/outline-style changes that have no
  rendering but use different values, like from
  outline-width:1px;outline-style:none to
  outline-width:0px;outline-style:dotted, on nsStyleOutline
* -moz-stack-sizing on nsStyleXUL (a bug that we don't return any
  hint here currently?)
* mColumnRuleWidth and mTwipsPerPixel on nsStyleColumn
* m{Attachment,Clip,Origin,Repeat,Position,Image,Size}Count on
  nsStyleBackground
* clip with different zero-sized values (since it's compared with
  IsEqualInterior), mOriginalDisplay, mOriginalFloats,
  mix-blend-mode (bug 902642) and all of the transition-* and
  animation-* properties on nsStyleDisplay
* mUserInput value changes not involving 'none', and mUserFocus on
  nsStyleUserInterface

Maybe we don't need to check for mTwipsPerPixel, since we'll be restyling everything in the document if we change full page zoom.

Comment 2

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #0)
>  * we need to check there aren't any style computation issues where
>    grandchild style contexts look at their grandparents during
>    computation.  If there are, we'd probably need to set bits to
>    indicate that.

Three of these I noticed:

* explicitly inherited align-self:auto
* any explicitly inherited reset properties on a ::first-line
* the generic font stuff (which I don't understand) that traverses
  right up the chain of nsStyleContext parents in
  nsRuleNode::SetGenericFont

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #1)
> * -moz-stack-sizing on nsStyleXUL (a bug that we don't return any
>   hint here currently?)

That's bug 492239.

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #1)
> * -moz-system-font changes on nsStyleFont

I think we should treat this the same as a font-family name change, rather than a no-op change.

Comment 5

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #0)
>  * we need to check there aren't any style computation issues where
>    grandchild style contexts look at their grandparents during
>    computation.  If there are, we'd probably need to set bits to
>    indicate that.

Since we need to store a new bit on style contexts, this depends on bug 773296 part 3, which expands nsStyleContext::mBits to a uint64_t.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

This
  (a) renames nsChangeHint_BorderStyleNoneChange to nsChangeHint_NeutralChange,
  (b) returns a new nsChangeHint_NeutralChange from CalcDifference methods for
      differences in struct data which we currently return NS_STYLE_HINT_NONE
      for, and
  (c) sets a bit on style contexts indicating that it used the grandparent (or
      some further ancestor) in computing some style.

Actual style context tree patching yet to come.

Comment 7

[Benoit Girard (:BenWa)]

I can help verifying the improvements this has towards fixing the style flush issues when settings a 'transform' transition on ffos. Needinfo me when you have a prototype I can test.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.2)

Since the last patch we
  (a) record on a style context if it shares some data (just nsStyleCoord::Calc
      objects currently) with a parent,
  (b) log a bunch of information about the restyling process (which might or
      might not be useful to keep around), and
  (c) do the actual style context parent patching when we're restyling with
      nsRestyleHint(0) or eRestyle_Self, we detect no style difference in the
      newly computed style, and we think it's safe.

This causes a to do more work than we used to, in these cases:
  (1) If we're copying a style context from a previous continuation, we still
      call CalcStyleDifferences between the copied style context and the
      frame's old style context.  There should be a way to remember the result
      of this from when we just restyled the previous frame of the
      continuation.  Maybe that's something to solve at the same time as bug
      918064 mentioned in ElementRestyler::Restyle.
  (2) In RestyleTracker::AddPendingRestyle, we no longer consider an
      eRestyle_Self restyle as a new restyle root, since we can't know whether,
      when we get to restyle it, if we'll end up restyling the whole subtree.
      The upshot is a bit more searching up the tree for a restyle root bit
      when we call AddPendingRestyle with eRestyle_Subtree.  We could try to
      be more clever about tracking this but it's probably not worth it.
  (3) In nsStyleContext::CalcStyleDifference we call CalcDifference where we
      were previously going to skip it, so that we can indicate whether there
      were any changes at all, even if they were already covered by
      aParentHintsNotHandledForDescendants.  If we wanted, we could add a
      HasAnyDifference(const nsStyleContext&) to style structs so that they
      could return early without computing the exact change hints, which we
      don't need in this case.

One other thing that the style context parent patching required was updating the cached style context pointers on the descendant style contexts, if they happened to inherit them from the parent we're moving away from.  Currently I'm traversing the entire subtree to do this patching, but I can make that stop earlier.  Same for checking whether we need to patch the old style context parent; I'm looking at the whole frame subtree to see whether there are other style contexts that need their parent updated, but I think we can stop at one level.

Still a bunch of assertions to resolve, though at least we're not crashing now.

https://tbpl.mozilla.org/?tree=Try&rev=193ef63efddd

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> One other thing that the style context parent patching required was updating
> the cached style context pointers on the descendant style contexts, if they
> happened to inherit them from the parent we're moving away from.

"cached style struct pointers", I meant to say.

Comment 10

[Dietrich Ayala (:dietrich)]

Release management: I'm renominating this for blocking 1.3. There's no patch, the win seems theoretical thus far, and there's no activity on this or any of the dependent bugs in the last 17 days.

Comment 11

[Cameron McCormack (:heycam)]

What is the timeframe for 1.3?  I have been working on this the last few days.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Cameron McCormack (:heycam) from comment #11)
> What is the timeframe for 1.3?  I have been working on this the last few
> days.

Cameron, please continue working on this, while we're sorting out the blocking status.  BenWa, do we have bugs that depend on this that are not listed as blocked by it?

Comment 13

[Benoit Girard (:BenWa)]

There's bug 939460 which is itself a tracking bug. I believe this bugs affects all apps that rely on async transaction.

Comment 14

[Dietrich Ayala (:dietrich)]

1.3 is already feature frozen, and is in stabilization period. What's the level of risk with this set of changes?

Comment 15

[Cameron McCormack (:heycam)]

It's going to be moderate, I'd say.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.3)

Since the last patch we:

  (a) now do the style context parent patching by looking at children of the
      "replaced" style context, rather than traversing down the frame tree
  (b) do all of the style context parent patching at the end of a call to
      ElementRestyler::Restyle, to avoid restyles of children to think that
      there was no change since the parent (eagerly set to the new parent)
      seemed like it hadn't changed
  (c) add a bunch more cases where we don't perform the optimization, such as
      when a style context is shared amongst siblings/cousins, when the style
      context is for a pseudo, when we're a child of an nsTableFrame, when
      the new style context points to a different rule node, when the style
      context is now / no longer is a link context or is visited, or if there
      is visited style involved (though I hope to not have to deoptimize when
      there is visited style)
  (d) stop traversing the style context tree while remapping any cached data
      when we know no descendant style contexts could possibly have inherited
      that cached data

and a bunch of failures have been fixed.  There are still some cases where we're not restyling when we should, which I'm looking in to.  Latest try run is here:

  https://tbpl.mozilla.org/?tree=Try&rev=18e86682eb9e

Despite all the orange, it's probably far enough along to be tested.  benwa, do you want to try it out?

I'm not seeing any Talos gains, but I have no idea whether any of our tests are really geared towards dynamic style changes.

Comment 17

[Cameron McCormack (:heycam)]

Though I have verified that the case we're interested in, where we update the transform of an element and expect not to recreate style contexts for all its descendants, is working.

Comment 18

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

(In reply to Cameron McCormack (:heycam) from comment #16)
>   (a) now do the style context parent patching by looking at children of the
>       "replaced" style context, rather than traversing down the frame tree

Why?  All other reresolution code uses the frame tree... and this means, as you point out in (c), that it now doesn't get along with sibling sharing, which seems like a significant problem.

>   (b) do all of the style context parent patching at the end of a call to
>       ElementRestyler::Restyle, to avoid restyles of children to think that
>       there was no change since the parent (eagerly set to the new parent)
>       seemed like it hadn't changed

I'm not sure I follow this.

>   (c) add a bunch more cases where we don't perform the optimization, such as
>       when a style context is shared amongst siblings/cousins, when the style
>       context is for a pseudo, when we're a child of an nsTableFrame, when
>       the new style context points to a different rule node, when the style
>       context is now / no longer is a link context or is visited, or if there
>       is visited style involved (though I hope to not have to deoptimize when
>       there is visited style)

When which new style context points to a different rule node?  If the child (i.e., the one you're considering parent-swapping instead of replacing), that's fine.

Comment 19

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #18)
> (In reply to Cameron McCormack (:heycam) from comment #16)
> >   (a) now do the style context parent patching by looking at children of the
> >       "replaced" style context, rather than traversing down the frame tree
> 
> Why?  All other reresolution code uses the frame tree... and this means, as
> you point out in (c), that it now doesn't get along with sibling sharing,
> which seems like a significant problem.

I wanted to avoid traversing all of the frames in the subtree, looking for parent style context pointers to update.  I don't think there is a good way to know when to stop looking down the frame tree, without replicating all of the logic that determines style context providers.

> >   (b) do all of the style context parent patching at the end of a call to
> >       ElementRestyler::Restyle, to avoid restyles of children to think that
> >       there was no change since the parent (eagerly set to the new parent)
> >       seemed like it hadn't changed
> 
> I'm not sure I follow this.

Say we start with this tree of style contexts (and assume the frame tree matches the style context tree):

...A
   |
   +--B
      |
      +--C
      |
      +--D

Now let's say that we begin with two pending restyles: B with eRestyle_Self, and C with nsRestyleHint(0).  So first we restyle B:

...A
   |
   +--B
   |  |
   |  +--C
   |  |
   |  +--D
   |
   +--B'


We find that its styles haven't significantly changed, so we patch C and D to point to the new parent:

...A
   |
   +--B
   |
   +--B'
      |
      +--C
      |
      +--D

B will then go away as we drop any references to it.

Now we come to process the second pending restyle, which was for C with nsRestyleHint(0).  That gets us in to nsStyleSet::ReparentStyleContext, which exits early because it looks at C's parent style context and notices that it's the same as the aNewParentContext that was passed in.  I can't actually remember now what particular brokenness this caused.  It might have been something like :hover styles not being applied.

> >   (c) add a bunch more cases where we don't perform the optimization, such as
> >       when a style context is shared amongst siblings/cousins, when the style
> >       context is for a pseudo, when we're a child of an nsTableFrame, when
> >       the new style context points to a different rule node, when the style
> >       context is now / no longer is a link context or is visited, or if there
> >       is visited style involved (though I hope to not have to deoptimize when
> >       there is visited style)
> 
> When which new style context points to a different rule node?  If the child
> (i.e., the one you're considering parent-swapping instead of replacing),
> that's fine.

No, I was looking at the style context that got replaced by one with same styles.

Comment 20

[Andrew Overholt [:overholt]]

Let's drop this from the 1.3 blocking nom list but request aurora uplift approval when we have a patch we'd like this in 1.3.

Comment 21

[Benoit Girard (:BenWa)]

Sorry for the delay in trying out this patch. I'm aiming to try it this week.

Comment 22

[Cameron McCormack (:heycam)]

Cool.  I'm going to have to make some broader changes to the approach, after discussions with dbaron, but it should still give the same kinds of performance improvements (if we do see some), so I think you're safe to try this patch still.

Comment 23

[Benoit Girard (:BenWa)]

(In reply to Cameron McCormack (:heycam) from comment #22)
> Cool.  I'm going to have to make some broader changes to the approach, after
> discussions with dbaron, but it should still give the same kinds of
> performance improvements (if we do see some), so I think you're safe to try
> this patch still.

Do you have a rebase? I'm getting some heavy conflicts and fuzzying.

Comment 24

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.3), rebased

Comment 25

[Benoit Girard (:BenWa)]

Here's a profile with when panning the homescreen. We're still seeing 50ms spent in restyle:
http://people.mozilla.org/~bgirard/cleopatra/#report=2e1f7031c8cade1fc2dae6416eff34ef6c2bd1fe

Trying again without the patch to get a frame of reference.

Comment 26

[Benoit Girard (:BenWa)]

Profile without the patch:
http://people.mozilla.org/~bgirard/cleopatra/#report=84ab50da242ec3697d109df1c67cdbe727b24433

This time we spent 70ms in restyle.

Are these the results you were expecting? 50ms still feels very heavy.

Comment 27

[Cameron McCormack (:heycam)]

That feels like too much.  Is the home screen panning done solely by setting element.style.transform?

Comment 28

[Cameron McCormack (:heycam)]

Benoit can you mail me a dump of the frame tree for the home screen?  I don't have any B2G debugging set up here and it would be good to know what's in there.

Comment 29

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

(In reply to Cameron McCormack (:heycam) from comment #16)
>   (c) add a bunch more cases where we don't perform the optimization, such as
>       when a style context is shared amongst siblings/cousins, when the style
>       context is for a pseudo, when we're a child of an nsTableFrame, when
>       the new style context points to a different rule node, when the style
>       context is now / no longer is a link context or is visited, or if there
>       is visited style involved (though I hope to not have to deoptimize when
>       there is visited style)

I think getting rid of the de-optimization for the sibling/cousin sharing case is probably needed to get good numbers.

(And I think that, in turn, depends on switching back to using the frame tree.)

Comment 30

[Cameron McCormack (:heycam)]

FWIW, I just got a proper B2G build setup with my Keon and made a modification to the homescreen app that did a requestAnimationFrame that changes the transform on the icon grid by 1px every callback.  With current Gecko trunk, I get 22 ms per frame, and with the (traversing the style context tree still) patch I get 17 ms.

Comment 31

[Benoit Girard (:BenWa)]

(In reply to Cameron McCormack (:heycam) from comment #30)
> FWIW, I just got a proper B2G build setup with my Keon and made a
> modification to the homescreen app that did a requestAnimationFrame that
> changes the transform on the icon grid by 1px every callback.  With current
> Gecko trunk, I get 22 ms per frame, and with the (traversing the style
> context tree still) patch I get 17 ms.

Last I looked last month I was seeing some cases acros gaia in the bugs I filed where we were spending about 100-200ms in the style flush. It would be good to confirm that you can reproduce those and test the patch against these.

Comment 32

[Cameron McCormack (:heycam)]

Was this something other than panning the home screen? Do you have any pointers?

Comment 33

[Benoit Girard (:BenWa)]

(In reply to Cameron McCormack (:heycam) from comment #32)
> Was this something other than panning the home screen? Do you have any
> pointers?

There's also the lockscreen, app transition and settings app impacted (likely more but I stopped profiling there):
bug 939463 bug 939461 bug 939260

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Benoit Girard (:BenWa) from comment #33)
> (In reply to Cameron McCormack (:heycam) from comment #32)
> > Was this something other than panning the home screen? Do you have any
> > pointers?
> 
> There's also the lockscreen, app transition and settings app impacted
> (likely more but I stopped profiling there):
> bug 939463 bug 939461 bug 939260

There is also bug 978588 (notifications tray).

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I think we should block 1.5 on this.

For triagers: this bug likely affects most of the transitions between panels for apps. The more content you have, the more visible it is, so apps with big list, like contacts, sms or settings are likely affected.

Contacts has workaround the bug in Gaia already, by transitioning a fake panel with -moz-element, but its painful to maintain, and it's hard to use the same trick for some other apps, like the homescreen.

Also the homescreen panning is heavily impacted, on a simple profile (http://people.mozilla.org/~bgirard/cleopatra/#report=9d8af16c1a367e0e439027d15f164c3ade50a812) the time spent in the Layout::Flush is 272ms for 511ms of time taken processing things (like repaint). This prevent us to reach our 60 fps panning target for the homescreen.

Can we prioritize this for 1.5 ?

Comment 36

[Vincent Lin[:vilin]]

Attachment: jank.png

Just wanna know if this Bug is helpful for what I encountered. In Bug 980241, I tried to make vsync-triggered stuffs and make sure each reflow and composition has ~16ms budget.

In my profile, "ComputeStyleChangeFor" takes the major cost of reflow which makes this frame miss the next Vsync.

Comment 37

[Cameron McCormack (:heycam)]

(In reply to Vincent Lin[:vilin] from comment #36)
> Just wanna know if this Bug is helpful for what I encountered. In Bug
> 980241, I tried to make vsync-triggered stuffs and make sure each reflow and
> composition has ~16ms budget.

Yes it should help with that, depending on the particular content/changes you're making.  If it's something like the homescreen panning (just changing transform on an element that has a subtree of other elements underneath it) then it will help.

Comment 38

[Mason Chang [Inactive] [:mchang]]

Attachment: Rebased for mozilla-central

Here are two profiles of the homescreen panning:

Mix of fast/slow - https://people.mozilla.org/~bgirard/cleopatra/#report=0e857ffd1cacf07cad5ca3edc597ff696e8f2c6d

slow drag - https://people.mozilla.org/~bgirard/cleopatra/#report=9eba4e31085c23a152a793bce7df34d805e12ce0 - look after the large rasterize block which is the unlock

Compared to the profile in 944564, over the span of 4500 ms, we drop from a 78 ms flush to 31 ms flush with a slow drag.

Comment 39

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

I think it would help to keep detailed analysis of the homescreen in other bugs; this bug is about one piece of work that we're doing that's likely to help the homescreen and other things, and I'd prefer to focus this on that particular work rather than on analyzing more profiles.

Comment 40

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.4)

Here's a WIP patch that goes back to working on the frame tree instead of the style context tree.  The basic approach now is as follows:

* Whenever we restyle a frame, any style structs that are cached and owned by
  the new and old style context, and which compare as equal, are swapped, so
  that the new style context keeps the old style structs around, while the old
  style context (which will be deleted) will end up deleting the newly allocate
  style structs that had no style changes.

* Whenever we restyle a frame and detect that it had no style changes at all,
  and we find ourselves not in a tricky frame tree structure that's hard to
  handle, we let the frame keep the new style context (which due to the first
  point will contain cached, owned style structs from the old style context) and
  then move all of the style context children from the old style context to the
  new one.

One of the "tricky frame tree structure" cases I am still deoptimising is when there are pseudo-elements.  Unfortunately scrollable frames, as created by overflow:scroll or overflow:hidden, run into this case.  So for the specific case of the Gaia home screen app, I had to make the #icongrid element (the one whose transform is being modified while panning) overflow:-moz-hidden-unscrollable.  I'll keep looking to see if I can avoid deoptimising non-inherited style changes to scrollable frames.

Comment 41

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=b12dd04255ee for WIP (v0.4).  Still a bunch of orange, but it's generally working.

Comment 42

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.5)

Three or four remaining test failures.  https://tbpl.mozilla.org/?tree=Try&rev=83a3ec4d138a

Comment 43

[Cameron McCormack (:heycam)]

I've run out of time to get a fully working patch queue ready.  This is what is remaining to do:

1. Debug the remaining test failures: https://tbpl.mozilla.org/?tree=Try&rev=dfb68c5ba744

   There are a couple of "Why did this not get handled while processing mRestyleRoots" assertions.
   I am not sure whether these are new problems, or existing problems surfaced by the new way
   restyling happens.  Plus there a few cases where we're asserting that we're destroying a style
   struct that is still used somewhere in the style context tree.

   No failures in opt builds, but the style struct assertion failures really need to be sorted out
   before it's safe for landing.

2. Need to add more comments explaining the optimisation, and the reasons why we avoid the optimisation
   in certain cases.

3. I'd like to add some tests for the cases in #2.

4. I'd like to be more certain that I've got the right set of cases where we need to avoid the
   optimisation.

Still, I think the patch queue can be reviewed at this point.  I'll attach it in a moment.  Feel free to change r? to feedback? where appropriate.

Comment 44

[Cameron McCormack (:heycam)]

Attachment: Part 20: Stop the recursive restyling process when we detect it is safe to do so

Comment 45

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add a change hint that represents a style data change that requires no processing

Comment 46

[Cameron McCormack (:heycam)]

Attachment: Part 2: Return nsChangeHint_NeutralChange where there was a style data change and we currently return nsChangeHint(0)

Comment 47

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add a style context bit to represent whether it depends on style data from its grandparent or higher ancestor

Comment 48

[Cameron McCormack (:heycam)]

Attachment: Part 4: Record whether a style context references objects allocated by its parent

Comment 49

[Cameron McCormack (:heycam)]

Attachment: Part 5: Record whether a style context is shared

Comment 50

[Cameron McCormack (:heycam)]

Attachment: Part 6: Add helper function to check if a style context has any children with the NS_STYLE_USES_GRANDANCESTOR_STYLE bit

Comment 51

[Cameron McCormack (:heycam)]

Attachment: Part 7: Add helper function to compare cached struct pointer values on two style contexts

Comment 52

[Cameron McCormack (:heycam)]

Attachment: Part 8: Add ability to swap style context allocated objects between style structs

Comment 53

[Cameron McCormack (:heycam)]

Attachment: Part 9: Make nsStyleContext::CalcStyleDifference compare all structs and return a bitfield of which changed

Comment 54

[Cameron McCormack (:heycam)]

Attachment: Part 10: Record pending restyles of restyle root descendants and ensure we restyle them if we didn't get to them

Comment 55

[Cameron McCormack (:heycam)]

Attachment: Part 11: Add an eRestyle_Force restyle hint to indicate the entire subtree must be processed

Comment 56

[Cameron McCormack (:heycam)]

Attachment: Part 12: Record on an ElementRestyler whether all ancestor frames we restyled had shared style contexts

Comment 57

[Cameron McCormack (:heycam)]

Attachment: Part 13: Add expensive (commented out) debug checks that destroyed style structs aren't still used in the style context tree

Comment 58

[Cameron McCormack (:heycam)]

Attachment: Part 14: Add functions to move style contexts to a new parent

Comment 59

[Cameron McCormack (:heycam)]

Attachment: Part 15: Pass equal struct bitfield back out of ElementRestyler::CaptureChange

Comment 60

[Cameron McCormack (:heycam)]

Attachment: Part 16: Give ElementRestyler::RestyleSelf a return value that indicates how restyling should proceed

Comment 61

[Cameron McCormack (:heycam)]

Attachment: Part 17: Handle eRestyleResult_Stop by moving children of the old style context to the new one

Comment 62

[Cameron McCormack (:heycam)]

Attachment: Part 18: Add a function to swap style structs between style contexts

Comment 63

[Cameron McCormack (:heycam)]

Attachment: Part 19: Make RebuildAllStyleData use eRestyle_Force so that it does rebuild the whole style tree

Comment 64

[Cameron McCormack (:heycam)]

Attachment: Part 20: Stop the recursive restyling process when we detect it is safe to do so

Comment 65

[Cameron McCormack (:heycam)]

Attachment: enable style struct destruction checking

Use this patch to turn on the expensive style struct destruction check assertions.

Comment 66

[Cameron McCormack (:heycam)]

Comment on attachment 8413122 [details] [diff] [review]
Part 6: Add helper function to check if a style context has any children with the NS_STYLE_USES_GRANDANCESTOR_STYLE bit

It would be good if we didn't need to loop over the child style contexts here.  (We call HasChildThatUsesGrandancestorStyle in ElementRestyler::ComputeRestyleResultFromFrame, in the final patch.)  We can probably be cleverer about setting the style context bits to avoid this.

Comment 67

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

(In reply to Cameron McCormack (:heycam) (away 27 April - 1 June) from comment #43)
>    There are a couple of "Why did this not get handled while processing
> mRestyleRoots" assertions.
>    I am not sure whether these are new problems, or existing problems
> surfaced by the new way
>    restyling happens.

Might these have been bug 997506?

Comment 68

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #67)
> Might these have been bug 997506?

Rebased on top of inbound tip and pushed to try again to see: https://tbpl.mozilla.org/?tree=Try&rev=b5ef976e8319

The ssty-1.html reftest assertion at least only happens when I have the first two patches of my queue applied.  So it must be something to do with returning nsChangeHint_NeutralChange.

Comment 69

[Jet Villegas (inactive)]

This is expected to land after June 1. Please update blocker status as needed.

Comment 70

[Milan Sreckovic [:milan] (needinfo for best results)]

June 2nd would be Gecko 32 and Firefox OS 2.0, June 9th would be Gecko 33 and Firefox OS 2.1.  A week will make a big difference, as this appears to be large enough to raise eyebrows if it has to be uplifted. I'm not clear if we're close so that Cameron can start landing this once he's back, but just rebasing is going to take some time.  David, can all these patches be reviewed, or should we wait for Cameron to be back?

Comment 71

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

Comment on attachment 8413117 [details] [diff] [review]
Part 1: Add a change hint that represents a style data change that requires no processing

I'm inclined to suggest that you not make these two changes:

>@@ -178,30 +194,32 @@ inline bool NS_IsHintSubset(nsChangeHint
>           nsChangeHint_UpdateEffects | \
>           nsChangeHint_UpdateOpacityLayer | \
>           nsChangeHint_UpdateOverflow | \
>           nsChangeHint_UpdatePostTransformOverflow | \
>           nsChangeHint_ChildrenOnlyTransform | \
>           nsChangeHint_RecomputePosition | \
>           nsChangeHint_AddOrRemoveTransform | \
>           nsChangeHint_BorderStyleNoneChange | \
>+          nsChangeHint_NeutralChange | \
>           nsChangeHint_NeedReflow | \
>           nsChangeHint_ClearAncestorIntrinsics)
> 
> inline nsChangeHint NS_HintsNotHandledForDescendantsIn(nsChangeHint aChangeHint) {
>   nsChangeHint result = nsChangeHint(aChangeHint & (
>     nsChangeHint_UpdateTransformLayer |
>     nsChangeHint_UpdateEffects |
>     nsChangeHint_UpdateOpacityLayer |
>     nsChangeHint_UpdateOverflow |
>     nsChangeHint_UpdatePostTransformOverflow |
>     nsChangeHint_ChildrenOnlyTransform |
>     nsChangeHint_RecomputePosition |
>     nsChangeHint_AddOrRemoveTransform |
>-    nsChangeHint_BorderStyleNoneChange));
>+    nsChangeHint_BorderStyleNoneChange |
>+    nsChangeHint_NeutralChange));
> 
>   if (!NS_IsHintSubset(nsChangeHint_NeedDirtyReflow, aChangeHint) &&
>       NS_IsHintSubset(nsChangeHint_NeedReflow, aChangeHint)) {
>     // If NeedDirtyReflow is *not* set, then NeedReflow is a
>     // non-inherited hint.
>     NS_UpdateHint(result, nsChangeHint_NeedReflow);
>   }
> 

since the not-handled-for-descendants case involves deoptimizing in some ways, and since no handling is needed for these hints, they are handled for descendants.

(I'm also not sure why it matters given the CalcStyleDifference change, but despite that I'd rather limit the not-handled cases to the minimum we need.)

r=dbaron with that

Comment 72

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

Comment on attachment 8413118 [details] [diff] [review]
Part 2: Return nsChangeHint_NeutralChange where there was a style data change and we currently return nsChangeHint(0)

In nsStyleBackground::CalcDifference, no need to check mImageCount since
it's already checked by the loop above (which always sets
hasVisualDifference when they differ).

Why do you need the operator== on nsTransition and nsAnimation?  Aren't
the implicit ones fine?  (Or is there an issue ensuring mUnknownProperty
is null when it should be?  If so, we should fix that.)

In nsStyleDisplay::CalcDifference, maybe share more code by using
a variable called transformHint for the properties that might set
a real hint or NeutralChange, and then do:
  if (transformHint) {
    if (HasTransformStyle()) {
      NS_UpdateHint(hint, transformHint);
    } else {
      NS_UpdateHint(hint, nsChangeHint_NeutralChange);
    }
  }

In nsStyleDisplay::CalcDifference, should you just use IsEqualEdges
on mClip, since its size could change with one side remaining 0?


A bunch of these places (e.g., in nsStyleFont, failure to check
mTwipsPerPixel in nsStyleColumn) might actually be bugs.  Maybe at
least add FIXME comments, and maybe file a bug?


r=dbaron

Comment 73

[Jason Smith [:jsmith]]

We're past FL & this reads off as feature work, so this should probably be kicked out of the blocking queue.

Comment 74

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

Comment on attachment 8413119 [details] [diff] [review]
Part 3: Add a style context bit to represent whether it depends on style data from its grandparent or higher ancestor

nsStyleContext.h:
+  // Does this style context, or any of its descendants, have any style values
+  // that were computed based on its grandparent style context or any of the
+  // grandparent's ancestors?
+  bool UsesGrandancestorStyle() const
+    { return !!(mBits & NS_STYLE_USES_GRANDANCESTOR_STYLE); }

Presumably "its grandparent style context" means "this style context's
grandparent style context"?  If so, please change to that, since it's
clearer.


nsRuleNode::PropagateGandancestorBit should assert that
!aContextInheritedFrom || aContextInheritedFrom != aContext->GetParent().
(Given the way it's written, it walks all the way up to the top in
that case.  If that assertion fails, it should be fixed to not do that.)

And, actually, it looks like that assertion will fail because of
the caller in WalkRuleTree.  So something ought to be fixed here.
Same for the SetGenericFont caller, I think.  But the former probably
means pretty much every style context has the bit set.

nsRuleNode::PropagateGrandancestorBit seems a little bit pessimistic
in the cases where aContextInheritedFrom is null; maybe it could bail
out in that case too, rather than setting bits almost up to the root?

Comment 75

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

Comment on attachment 8413120 [details] [diff] [review]
Part 4: Record whether a style context references objects allocated by its parent

In nsStyleSides::SharesStyleContextAllocatedObjectsWith:
>+          mValues[i].mPointer != aOther.mValues[i].mPointer) {

That should be == rather than !=.

Likewise for nsStyleCorners.


I'm a little worried about the maintainability here.  What happens
if somebody forgets to update the nsStyleStruct methods here when
adding a new nsStyleCoord member?  Is there a risk of exploitable
crashes?

I'm inclined to think we should give nsStyleCoord a destructor and
make the Calc objects reference-counted, though that can be a
FOLLOWUP BUG if this works for now.  But we should do it quickly.
If you want to do it as part of this bug that's fine too.


You missed:
  mBorderImageSource
  nsStyleGridTemplate::mMinTrackSizingFunctions
  nsStyleGridTemplate::mMaxTrackSizingFunctions
  mMaxHeight
  mFlexBasis
  mGridAutoColumnsMin
  mGridAutoColumnsMax
  mGridAutoRowsMin
  mGridAutoRowsMax
  mMarkerOffset (assertion)

The trick of not using the "aOther." on the assertions is clever,
but should perhaps use a comment every single time it happens.

But you missed using that trick here:
>+    MOZ_ASSERT(!mZIndex.SharesStyleContextAllocatedObjectsWith(aOther.mZIndex));


r=dbaron with that, I guess

Comment 76

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

Comment on attachment 8413121 [details] [diff] [review]
Part 5: Record whether a style context is shared

r=dbaron

(out of bits for now!)

Comment 77

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

Comment on attachment 8413122 [details] [diff] [review]
Part 6: Add helper function to check if a style context has any children with the NS_STYLE_USES_GRANDANCESTOR_STYLE bit

Maybe this static helper should be private?

r=dbaron

Comment 78

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

Comment on attachment 8413123 [details] [diff] [review]
Part 7: Add helper function to compare cached struct pointer values on two style contexts

I think the current code is consistent about using the term "dependent" for caching structs down the rule tree, and "inherited" for caching structs down the style context tree (since it represents inheritance).

So I think I'd prefer calling the second method HasCachedInheritedStyleData, and updating the comment to match, if you think that makes sense.

Also, please update the commit message to say you're adding two methods.

Comment 79

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

Comment on attachment 8413124 [details] [diff] [review]
Part 8: Add ability to swap style context allocated objects between style structs

This makes me think we should take the alternative approach for patch 4 and just change the allocations setup now.

Comment 80

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

Comment on attachment 8413125 [details] [diff] [review]
Part 9: Make nsStyleContext::CalcStyleDifference compare all structs and return a bitfield of which changed

In nsStyleContext.h, please document that CalcStyleDifference sets the
equal bit when the old context didn't have the struct.

I don't think it's worth going to all this trouble to avoid calling NS_UpdateHint; you should just combine these two branches:

>+      } else if (compare ||                                                   \
>+                 (NS_SubtractHint(maxDifference,                              \
>+                                  maxDifferenceNeverInherited) &              \
>+                  aParentHintsNotHandledForDescendants)) {                    \
>+        nsChangeHint difference =                                             \
>+            this##struct_->CalcDifference(*other##struct_);                   \
>+        NS_ASSERTION(NS_IsHintSubset(difference, maxDifference),              \
>+                     "CalcDifference() returned bigger hint than "            \
>+                     "MaxDifference()");                                      \
>+        NS_UpdateHint(hint, difference);                                      \
>+        if (!difference) {                                                    \
>+          *aEqualStructs |= NS_STYLE_INHERIT_BIT(struct_);                    \
>+        }                                                                     \
>+      } else {                                                                \
>+        /* We still must call CalcDifference to see if there were any */      \
>+        /* changes so that we can set *aEqualStructs appropriately.   */      \
>+        nsChangeHint difference =                                             \
>+            this##struct_->CalcDifference(*other##struct_);                   \
>+        NS_ASSERTION(NS_IsHintSubset(difference, maxDifference),              \
>+                     "CalcDifference() returned bigger hint than "            \
>+                     "MaxDifference()");                                      \
>+        if (!difference) {                                                    \
>+          *aEqualStructs |= NS_STYLE_INHERIT_BIT(struct_);                    \
>+        }                                                                     \

which I think in turn means we can get rid of MaxDifference() and MaxDifferenceNeverInherited in a separate patch.

But the fact that you removed nsStyleContext::AssertStyleStructMaxDifferenceValid makes me think you went down that path before?  In any case, that removal should go along with the removal of MaxDifference itself and not be here.

r=dbaron with that, though if you still need the two branches I'd like to understand why.

Comment 81

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

Comment on attachment 8413126 [details] [diff] [review]
Part 10: Record pending restyles of restyle root descendants and ensure we restyle them if we didn't get to them

>   if (aData->mRestyleHint & eRestyle_LaterSiblings) {
>     // Someone readded the eRestyle_LaterSiblings hint for this
>     // element.  Leave it around for now, but remove the other restyle
>     // hints and the change hint for it.  Also unset its root bit,
>     // since it's no longer a root with the new restyle data.
>     RestyleData newData;
>     newData.mChangeHint = nsChangeHint(0);
>     newData.mRestyleHint = eRestyle_LaterSiblings;
>+    newData.mDescendants.SwapElements(aData->mDescendants);
>     mPendingRestyles.Put(aElement, newData);
>     aElement->UnsetFlags(RootBit());
>     aData->mRestyleHint =
>       nsRestyleHint(aData->mRestyleHint & ~eRestyle_LaterSiblings);
>   } else {

I don't understand why you're making this change.  The descendants that
we need to ensure we restyled should be handled during handling of the
eRestyle_Self bit.  I think we want those descendants handled with aData
and not with newData.

(I'm not sure when we'd hit this code, though.)


In RestyleTracker::DoProcessRestyles, please comment that the order
of adding to mRestyleRoots and position in mRestyleRoots is important
for the tree-based coalescing to work correctly.  (I think you have it
right, but it's not obvious that it's important.)

In RestyleTracker::AddPendingRestyle, I don't see any reason to
move the mPendingRestyles.Put(aElement, existingData); line.  If it
needs to be moved for some reason, you should add a comment explaining
why.  (It seems better not to have to keep existingData on the stack
for longer.)

In RestyleTracker::AddPendingRestyle, right before the break out of the
loop, you should add a "cur = aElement", so that in that case you skip
the new chunk that you've added.

>+    if (cur != aElement && !(aRestyleHint & eRestyle_Subtree)) {

I don't see why you have this test of aRestyleHint here.  I could
understand wanting to test the restyle hint in curData once you get it,
but testing aRestyleHint seems wrong.  (I'm surprised this didn't cause
problems.  Or maybe it did.)

>+      RestyleData curData;
>+      // XXX We should avoid copying RestyleData::mDescendants around.
>+      mPendingRestyles.Get(cur, &curData);
>+      curData.mDescendants.AppendElement(aElement);
>+      mPendingRestyles.Put(cur, curData);

Use mPendingRestyles.GetEntry(cur), and then access its
->mData.mDescendants.AppendElement(aElement).  But you might need to
null-check it and PutEntry if it's not there -- although I actually
hope that you don't, at least with the above fix.

r=dbaron with that

Comment 82

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

Comment on attachment 8413127 [details] [diff] [review]
Part 11: Add an eRestyle_Force restyle hint to indicate the entire subtree must be processed

>-  if (childRestyleHint == eRestyle_Self) {
>-    childRestyleHint = nsRestyleHint(0);
>+  // If we are restyling this frame with eRestyle_Self, we restyle
>+  // children with nsRestyleHint(0).  But we pass the eRestyle_Force flag
>+  // down too.
>+  if ((childRestyleHint & ~eRestyle_Force) == eRestyle_Self) {
>+    childRestyleHint = nsRestyleHint(childRestyleHint & eRestyle_Force);
>   }

Could you just do:

  nsRestyleHint childRestyleHint =
    nsRestyleHint(aRestyleHint & (eRestyle_Subtree | eRestyle_Force));

(That's what I have in my tree in Bug 996796 patch 7, which is part of
adding additional hints like Self but weaker, and it's also simpler.)

We shouldn't get eLaterSiblings here, and you can assert that if you
want.  (Though I didn't in my patch, but perhaps should have.)

r=dbaron with that

Comment 83

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

Comment on attachment 8413128 [details] [diff] [review]
Part 12: Record on an ElementRestyler whether all ancestor frames we restyled had shared style contexts

>+  // Whether all ancestor frames we restyled have shared style contexts.
>+  bool mAncestorsAllHaveSharedStyle;

Maybe make the comment a little clearer that:
 * this frame is included
 * it's only counting ancestors up to the restyle root
?

r=dbaron

Comment 84

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

Comment on attachment 8413129 [details] [diff] [review]
Part 13: Add expensive (commented out) debug checks that destroyed style structs aren't still used in the style context tree

It might be nice to de-macro-ize this by just looping over the enum values, and adding an #ifdef DEBUG gStyleStructNames array that includes nsStyleStructList.h just to get the names of the structs (unless we have another such array already), and using nsPrintfCString for the assertion text.  (There's really nothing here that requires repeated code.)

Given that it's #ifdef DEBUG and then #if 0, perhaps not worth it, though?

Comment 85

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

Comment on attachment 8413130 [details] [diff] [review]
Part 14: Add functions to move style contexts to a new parent

The comments for MoveTo and MoveChildrenTo should explain that in
general these functions violate style context immutability, and they're
very low-level functions that should only be used after verifying that
many things are safe.

It should also either assert that aNewParent != mParent, or bail
at the beginning if they're equal.

It also might be good practice to AddRef aNewParent before calling
Release on mParent, although it's not actually an issue now.

In MoveChildrenTo, can these assertions:
>+    MOZ_ASSERT(!child->IsStyleIfVisited() || child->mParent == this);
actually just be:
>+    MOZ_ASSERT(child->mParent == this);

If not, why not?  Or did you mean && rather than ||?  (I suspect so.)

r=dbaron with those things addressed

Comment 86

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

Comment on attachment 8413132 [details] [diff] [review]
Part 16: Give ElementRestyler::RestyleSelf a return value that indicates how restyling should proceed

>+      // We assume RestyleSelf will never return eRestyleResult_Stop for
>+      // one continuation or ib sibling and another value for another.
>+
>+      NS_ASSERTION(result != eRestyleResult_Stop, "cannot stop restyling yet");
>+
>+      if (result == eRestyleResult_ContinueAndForce) {
>+        childRestyleHint = nsRestyleHint(childRestyleHint | eRestyle_Force);
>+      }

The comment at the top looks related to the assertion, but it seems more related to the test below it.  Maybe there should be an assertion asserting what the comment says, too?  (It also might be worth explaining why the comment is the case, even in the presence of things inside of ::first-line.)  But I have the distinct feeling I'm not looking a the final code here.

r=dbaron

Comment 87

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

Comment on attachment 8413133 [details] [diff] [review]
Part 17: Handle eRestyleResult_Stop by moving children of the old style context to the new one

>       // We assume RestyleSelf will never return eRestyleResult_Stop for
>       // one continuation or ib sibling and another value for another.

Yeah, you definitely need an assertion to check this.  (I'm not completely sure it's true.)

>+      // This assumes that the |newContext| has had its cached style
>+      // data modified so that it is safe to just move the children of
>+      // |oldContext| over to |newContext| without restyling the children.
>+      nsStyleContext* newContext = mFrame->StyleContext();
>+      oldContext->MoveChildrenTo(newContext);

I'm still working on understanding why we want to move the children of oldContext rather than moving it.

Comment 88

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

Comment on attachment 8413135 [details] [diff] [review]
Part 19: Make RebuildAllStyleData use eRestyle_Force so that it does rebuild the whole style tree

Instead of adding a second boolean, where we have two booleans used to set a bitfield, we should just pass the bitfield around.

That's actually what I have in https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/5e8e19d18b41/pass-restyle-hint-through which is part of my patch series in which I'm adding new restyle hints.  I should really just tro to get that in.

Comment 89

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

So part 19 should be redone on top of bug 1026768.

Comment 90

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

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #87)
> >+      // This assumes that the |newContext| has had its cached style
> >+      // data modified so that it is safe to just move the children of
> >+      // |oldContext| over to |newContext| without restyling the children.
> >+      nsStyleContext* newContext = mFrame->StyleContext();
> >+      oldContext->MoveChildrenTo(newContext);
> 
> I'm still working on understanding why we want to move the children of
> oldContext rather than moving it.

I guess I still haven't figured this out, though maybe I'm just misremembering the plan.

I'm also planning to hold off on reviewing patch 20 until it has more comments.

Comment 91

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #72)
> Comment on attachment 8413118 [details] [diff] [review]
> Why do you need the operator== on nsTransition and nsAnimation?  Aren't
> the implicit ones fine?  (Or is there an issue ensuring mUnknownProperty
> is null when it should be?  If so, we should fix that.)

There's no such thing as an implicit operator==, right?  Or am I misunderstanding?

Comment 92

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

(In reply to Cameron McCormack (:heycam) from comment #91)
> There's no such thing as an implicit operator==, right?  Or am I
> misunderstanding?

Oh, I guess not.  Never mind.

Comment 93

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #78)
> Comment on attachment 8413123 [details] [diff] [review]
> Part 7: Add helper function to compare cached struct pointer values on two
> style contexts
> 
> I think the current code is consistent about using the term "dependent" for
> caching structs down the rule tree, and "inherited" for caching structs down
> the style context tree (since it represents inheritance).
> 
> So I think I'd prefer calling the second method HasCachedInheritedStyleData,
> and updating the comment to match, if you think that makes sense.

As the cached structs could include both ones that came from the rule tree and which are dependent, and ones which are inherited down the style context tree, what about HasCachedNonOwnedStyleData instead?

Comment 94

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #80)
> Comment on attachment 8413125 [details] [diff] [review]
> Part 9: Make nsStyleContext::CalcStyleDifference compare all structs and
> return a bitfield of which changed
> 
> I don't think it's worth going to all this trouble to avoid calling
> NS_UpdateHint; you should just combine these two branches:
...
> But the fact that you removed
> nsStyleContext::AssertStyleStructMaxDifferenceValid makes me think you went
> down that path before?  In any case, that removal should go along with the
> removal of MaxDifference itself and not be here.
> 
> r=dbaron with that, though if you still need the two branches I'd like to
> understand why.

I do get more failures when I combine the two branches:

https://tbpl.mozilla.org/?tree=Try&rev=1ef920c54c82 (without combining)
https://tbpl.mozilla.org/?tree=Try&rev=bcfa721e4028 (with combining)

I'll try to find out why.

Comment 95

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #81)
> Use mPendingRestyles.GetEntry(cur), and then access its
> ->mData.mDescendants.AppendElement(aElement).  But you might need to
> null-check it and PutEntry if it's not there -- although I actually
> hope that you don't, at least with the above fix.

nsDataHastable publicly inherits from nsBaseHastable, but that privately inherits from nsTHashtable, which is where GetEntry is defined.  I'm not sure if exposing GetEntry on nsDataHashtable is the right thing to do.

Comment 96

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

Maybe better to expose a way to manipulate the value on nsDataHashtable, then?  Or maybe just don't worry about it for now.

Comment 97

[Cameron McCormack (:heycam)]

I could, but I feel like nsDataHashtable is intended for value types where you don't mind where that they are being copied, because they're small.  I'll leave it for now and maybe see if one of the other hashtable classes would work better later.

Comment 98

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #81)
> Comment on attachment 8413126 [details] [diff] [review]
> Part 10: Record pending restyles of restyle root descendants and ensure we
> restyle them if we didn't get to them
...
> >+    if (cur != aElement && !(aRestyleHint & eRestyle_Subtree)) {
> 
> I don't see why you have this test of aRestyleHint here.  I could
> understand wanting to test the restyle hint in curData once you get it,
> but testing aRestyleHint seems wrong.  (I'm surprised this didn't cause
> problems.  Or maybe it did.)

Yes that seems wrong.  I could actually make the check for eRestyle_Force instead (that must have been the kind of thing I was thinking of, before I actually introduced eRestyle_Force), but not sure it's worth it, as it's not going to be common for RestyleTracker::AddPendingRestyle to be called with eRestyle_Force and then have other calls to AddPendingRestyle before the restyles are processed.

Comment 99

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #84)
> Comment on attachment 8413129 [details] [diff] [review]
> Part 13: Add expensive (commented out) debug checks that destroyed style
> structs aren't still used in the style context tree
> 
> Given that it's #ifdef DEBUG and then #if 0, perhaps not worth it, though?

I'm going to say not worth it since, as I just found out, you can't pass a non-string-literal to MOZ_ASSERT, so I can't the message including the struct name to appear on tbpl.

Comment 100

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add a style context bit to represent whether it depends on style data from its grandparent or higher ancestor. (v2)

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #74)
> Comment on attachment 8413119 [details] [diff] [review]
> Part 3: Add a style context bit to represent whether it depends on style
> data from its grandparent or higher ancestor
> 
> nsStyleContext.h:
> +  // Does this style context, or any of its descendants, have any style
> values
> +  // that were computed based on its grandparent style context or any of the
> +  // grandparent's ancestors?
> +  bool UsesGrandancestorStyle() const
> +    { return !!(mBits & NS_STYLE_USES_GRANDANCESTOR_STYLE); }
> 
> Presumably "its grandparent style context" means "this style context's
> grandparent style context"?  If so, please change to that, since it's
> clearer.

Done.

> nsRuleNode::PropagateGandancestorBit should assert that
> !aContextInheritedFrom || aContextInheritedFrom != aContext->GetParent().
> (Given the way it's written, it walks all the way up to the top in
> that case.  If that assertion fails, it should be fixed to not do that.)

Done.

> And, actually, it looks like that assertion will fail because of
> the caller in WalkRuleTree.  So something ought to be fixed here.
> Same for the SetGenericFont caller, I think.  But the former probably
> means pretty much every style context has the bit set.

Fixed the two call sites.

> nsRuleNode::PropagateGrandancestorBit seems a little bit pessimistic
> in the cases where aContextInheritedFrom is null; maybe it could bail
> out in that case too, rather than setting bits almost up to the root?

Done.

Comment 101

[Cameron McCormack (:heycam)]

Comment on attachment 8413120 [details] [diff] [review]
Part 4: Record whether a style context references objects allocated by its parent

No longer needed.

Comment 102

[Cameron McCormack (:heycam)]

Comment on attachment 8413124 [details] [diff] [review]
Part 8: Add ability to swap style context allocated objects between style structs

Neither is this.

Comment 103

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

(In reply to Cameron McCormack (:heycam) from comment #99)
> I'm going to say not worth it since, as I just found out, you can't pass a
> non-string-literal to MOZ_ASSERT, so I can't the message including the
> struct name to appear on tbpl.

Well, you could use NS_ABORT_IF_FALSE (the new and shiny thing isn't always better), but it might still not be worth it.

Comment 104

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #87)
> Comment on attachment 8413133 [details] [diff] [review]
> Part 17: Handle eRestyleResult_Stop by moving children of the old style
> context to the new one
> 
> >       // We assume RestyleSelf will never return eRestyleResult_Stop for
> >       // one continuation or ib sibling and another value for another.
> 
> Yeah, you definitely need an assertion to check this.  (I'm not completely
> sure it's true.)

You have me wondering about this now.  (Though I added the assertion and found it doesn't trigger on a full test run.)  I am struggling to construct an example where you would run into this.

By the way I make this assumption because currently I do the style struct swapping inside RestyleElement (right before the SetStyleContext call), and if the different continuations could return different RestyleResults, we'd want to look at all of the results before going ahead and doing the swapping/setting.

Comment 105

[Cameron McCormack (:heycam)]

Attachment: Part 19: Make RebuildAllStyleData use eRestyle_Force so that it does rebuild the whole style tree. (v2)

Comment 106

[Cameron McCormack (:heycam)]

(Question in comment 93.)

Comment 107

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

(In reply to Cameron McCormack (:heycam) from comment #93)
> (In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from
> comment #78)
> > Comment on attachment 8413123 [details] [diff] [review]
> > Part 7: Add helper function to compare cached struct pointer values on two
> > style contexts
> > 
> > I think the current code is consistent about using the term "dependent" for
> > caching structs down the rule tree, and "inherited" for caching structs down
> > the style context tree (since it represents inheritance).
> > 
> > So I think I'd prefer calling the second method HasCachedInheritedStyleData,
> > and updating the comment to match, if you think that makes sense.
> 
> As the cached structs could include both ones that came from the rule tree
> and which are dependent, and ones which are inherited down the style context
> tree, what about HasCachedNonOwnedStyleData instead?

Hmmm.  Do we actually cache structs that are on *this* style context's rule node on the style context as well?  If so, then yes.  But if you're talking about style contexts that are propagated through both the rule tree and the style context tree, then those styles have in fact been inherited, and inherited is a fine term.  (Propagation in the style context tree represents actual CSS inheritance; propagation in the rule tree only represents style data not overridden by a more specific rule.)

Comment 108

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

(In reply to Milan Sreckovic [:milan] from comment #70)
> June 2nd would be Gecko 32 and Firefox OS 2.0, June 9th would be Gecko 33
> and Firefox OS 2.1.  A week will make a big difference, as this appears to
> be large enough to raise eyebrows if it has to be uplifted. I'm not clear if
> we're close so that Cameron can start landing this once he's back, but just
> rebasing is going to take some time.  David, can all these patches be
> reviewed, or should we wait for Cameron to be back?

I think it's still going to take a bit more time, but it should move faster now that both Cameron and I are back.  (Not sure if that answer is much use at this point, though.)

Comment 109

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #107)
> (In reply to Cameron McCormack (:heycam) from comment #93)
> > (In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from
> > comment #78)
> > > Comment on attachment 8413123 [details] [diff] [review]
> > > Part 7: Add helper function to compare cached struct pointer values on two
> > > style contexts
> > > 
> > > I think the current code is consistent about using the term "dependent" for
> > > caching structs down the rule tree, and "inherited" for caching structs down
> > > the style context tree (since it represents inheritance).
> > > 
> > > So I think I'd prefer calling the second method HasCachedInheritedStyleData,
> > > and updating the comment to match, if you think that makes sense.
> > 
> > As the cached structs could include both ones that came from the rule tree
> > and which are dependent, and ones which are inherited down the style context
> > tree, what about HasCachedNonOwnedStyleData instead?
> 
> Do we actually cache structs that are on *this* style context's rule
> node on the style context as well?  If so, then yes.

Oh, so we only do that for inherited structs:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#2667

so you're right it's always inherited style.

Comment 110

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8413117 [details] [diff] [review]
Part 1: Add a change hint that represents a style data change that requires no processing

Review of attachment 8413117 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsChangeHint.h
@@ +124,5 @@
>     */
> +  nsChangeHint_UpdateTextPath = 0x20000,
> +
> +  /**
> +   * A hint reflecting that style data changed which no change handling

I fail to understand this sentence

Comment 111

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

ah, it needs s/which/with/

Comment 112

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #87)
> >+      // This assumes that the |newContext| has had its cached style
> >+      // data modified so that it is safe to just move the children of
> >+      // |oldContext| over to |newContext| without restyling the children.
> >+      nsStyleContext* newContext = mFrame->StyleContext();
> >+      oldContext->MoveChildrenTo(newContext);
> 
> I'm still working on understanding why we want to move the children of
> oldContext rather than moving it.

By this point, the compared-equal style structs will already have been swapped between oldContext and newContext.  So children that have cached inherited structs need to point to newContext as that's where they're now inheriting them from.  The alternative is to notice at the end of RestyleSelf that we are going to return eRestyleResult_Stop and to revert the style struct swapping.  (I *think* that was the only reason for keeping the new compared-equal style context.)

If you are worried about the work involved in calling RemoveChild/AddChild for each child style context, we could add an nsStyleContext::SwapChildren(nsStyleContext*) method that skips most of that work by swapping mChild/mEmptyChildren between the two and updating reference counts on the two parents.

Comment 113

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #112)
> By this point, the compared-equal style structs will already have been
> swapped between oldContext and newContext.  So children that have cached
> inherited structs need to point to newContext as that's where they're now
> inheriting them from.  The alternative is to notice at the end of
> RestyleSelf that we are going to return eRestyleResult_Stop and to revert
> the style struct swapping.  (I *think* that was the only reason for keeping
> the new compared-equal style context.)

This was actually more of a problem with earlier versions of the patch queue, where the CalcStyleDifference call also took care of the style struct swapping, and so there would be a need to swap them back at the end of RestyleSelf.  But now we leave the struct swapping right until the end when we know we're going to return eRestyleResult_Stop.  So it probably is possible to keep the old style context.  I'll take a look.

Comment 114

[Cameron McCormack (:heycam)]

Attachment: Part 21: Fix issue where not all pending-restyle descendants are restyled.

I found a problem with the way we track the descendants of a restyle root that we must ensure we end up restyling.  If you have a tree structure like this:

  A
  +--B
     +--C
        +--D
           +--E

and we add pending restyles for A, C, E, in that order, we'll end up with the RestyleData listing C as a descendant of A and E as a descendant of C.  If our restyling process stops at element D, we'll notice that C needed a restyle but we already handled it, but we don't know about E.  That's because we only check RestyleData::mDescendants on the element we pass in to ProcessOneRestyle.  So instead we need to check mDescendants on each element whose RestyleData we grab out in ElementRestyler::Restyle, and let the RestyleTracker know about those.

Unfortunately we can't just do this work in GetRestyleData itself, since we need to append those descendant elements to our list of elements to check only after we have restyled the subtree.  This is to keep the "ancestors come after descendants" ordering.

I'm not that happy about the extra array copying we're doing now -- let me know if you have any better ideas.

I also discovered that we really needed to be holding strong references to the elements in the descendants arrays, since the elements can go away between restyle event postings.

Comment 115

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

Comment on attachment 8452038 [details] [diff] [review]
Part 21: Fix issue where not all pending-restyle descendants are restyled.

Since the point where you call RecordePotentialUnrestyledDescendents is after your call (if there's going to be one) to RestyleChildren, I think you can just do the RestyleBit() check right then and append to mRestyleRoots immediately, instead of appending everything in the array.

In other words, it seems like the loop in DoProcessRestyles can be moved into RecordPotentialUnrestyledDescendants, and mPotentialUnrestyledDescendants can go away.

I also think you shouldn't need to bother in the RestyleUndisplayedChildren case, since if there are no frames, there's no need to restyle.

Comment 116

[Cameron McCormack (:heycam)]

Attachment: Part 21: Fix issue where not all pending-restyle descendants are restyled. (v2)

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #115)
> Since the point where you call RecordePotentialUnrestyledDescendents is
> after your call (if there's going to be one) to RestyleChildren, I think you
> can just do the RestyleBit() check right then and append to mRestyleRoots
> immediately, instead of appending everything in the array.

Oh yeah, good point.

> I also think you shouldn't need to bother in the RestyleUndisplayedChildren
> case, since if there are no frames, there's no need to restyle.

Ah because we don't actually go recursively restyling down there.  OK.

Comment 117

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

Comment on attachment 8452763 [details] [diff] [review]
Part 21: Fix issue where not all pending-restyle descendants are restyled. (v2)

>+        MOZ_ASSERT(undisplayedRestyleData.mDescendants.IsEmpty(),
>+                   "do we need to handled mDescendants here?");

I don't think you want this assertion; I think it would fire.  I think a comment explaining that there's no need to restyle should be sufficient.



I'm not crazy about switching to strong references.  Do you know why we didn't need them before?  Also, is there a risk it means that the restyle manager needs to be cycle collected?

r=dbaron with that, although I'm still curious about the issues in my last paragraph

Comment 118

[Cameron McCormack (:heycam)]

I'm pretty sure RestyleData::mDescendants needs to have strong references, since anything can happen between AddPendingRestyle calls.  It seemed easier to have AddRestyleRootsIfAwaitingRestyle take the same type as RestyleData::mDescendants; and since we're using SwapElements to grab out those descendants into the local variable in ElementRestyler::Restyle, we shouldn't be thrashing the refcounts too much.

Is it possible for the RestyleTracker to be tracking some pending restyles, for the document to stop being displayed, and then the RestyleTracker is not cleared out?  If so there might be a problem.  But given RestyleTracker::mRestyleRoots already is an array of strong element references, wouldn't this be a problem already (and we probably would have noticed it)?  If it is an actual problem we could clear out the RestyleTracker's mPendingRestyles and mRestyleRoots once the document has lost its pres shell.

Comment 119

[Cameron McCormack (:heycam)]

Comment on attachment 8413134 [details] [diff] [review]
Part 18: Add a function to swap style structs between style contexts

No longer needed.

Comment 120

[Cameron McCormack (:heycam)]

Attachment: Part 17: Handle eRestyleResult_Stop by keeping the old style context and moving it to its new parent. (v2)

So yes it was OK for us to keep the old style context on the frame when we detect no style data changes, and then to move it to its new parent.

Comment 121

[Cameron McCormack (:heycam)]

Attachment: Part 20: Stop the recursive restyling process when we detect it is safe to do so. (v2)

I was able to remove a few cases where we deoptimize here, and I've added comments for the remaining ones.

Comment 122

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=1bc68d2356d7

Comment 123

[Cameron McCormack (:heycam)]

The only thing I haven't addressed is why we can't merge the two branches mentioned in comment 94.  I merging those branches exposes some underlying issue where we rely on the optimisation that prevents certain style structs from being computed.  I haven't dug deep enough to work out what that problem is.  I suggest we get these patches landed first and work out why and then simplify that code in CalcStyleDifference later.

Comment 124

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

Comment on attachment 8452893 [details] [diff] [review]
Part 17: Handle eRestyleResult_Stop by keeping the old style context and moving it to its new parent. (v2)

I don't see what makes this leave the old style context on the frame -- RestyleSelf sets the new style context, no?

(I'm also suspicious about comment 119 -- could you explain why it's no longer needed?)

Maybe it's worth just trying to get this in as you had this patch before, and then trying to do the one remaining level of optimization later?  (Sorry, perhaps I should have said that the laste time around.)

Comment 125

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

(In reply to Cameron McCormack (:heycam) from comment #123)
> The only thing I haven't addressed is why we can't merge the two branches
> mentioned in comment 94.  I merging those branches exposes some underlying
> issue where we rely on the optimisation that prevents certain style structs
> from being computed.  I haven't dug deep enough to work out what that
> problem is.  I suggest we get these patches landed first and work out why
> and then simplify that code in CalcStyleDifference later.

That sounds fine as long as you've dropped the removal of nsStyleContext::AssertStyleStructMaxDifferenceValid that was in part 9.

Comment 126

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #124)
> Comment on attachment 8452893 [details] [diff] [review]
> Part 17: Handle eRestyleResult_Stop by keeping the old style context and
> moving it to its new parent. (v2)
> 
> I don't see what makes this leave the old style context on the frame --
> RestyleSelf sets the new style context, no?

Oh yes I guess that comment isn't right there.  It should say something like "if we get eRestyleResult_Stop, we assume that the old style context was left on the frame".  Maybe I should even assert it there.  The part 20 changes make RestyleSelf leave the old style context there.

> (I'm also suspicious about comment 119 -- could you explain why it's no
> longer needed?)

I believe we only needed to swap the style structs between the two style contexts when we were leaving the new style context (with equal style data to the old) on the frame, and moving its children across to this new style context.  This ensured that cached inherited structs on those children were still valid.  But we deoptimise if we detect that the old style context's parent has any different cached struct pointer values from the new style context's parent.  So from this we should know that all of the cached structs on the old style context's children are still good (either they inherit structs owned by their parent (the old style context), in which case nothing has changed, or they inherit structs owned by an ancestor, and the restyling process didn't create new structs for them).

Does that sound right to you?

> Maybe it's worth just trying to get this in as you had this patch before,
> and then trying to do the one remaining level of optimization later? 
> (Sorry, perhaps I should have said that the laste time around.)

You might've said that to me and I forgot.  Let me know if you are happy with the above comments, and if not I can go back to the old patches.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #125)
> (In reply to Cameron McCormack (:heycam) from comment #123)
> > The only thing I haven't addressed is why we can't merge the two branches
> > mentioned in comment 94.  I merging those branches exposes some underlying
> > issue where we rely on the optimisation that prevents certain style structs
> > from being computed.  I haven't dug deep enough to work out what that
> > problem is.  I suggest we get these patches landed first and work out why
> > and then simplify that code in CalcStyleDifference later.
> 
> That sounds fine as long as you've dropped the removal of
> nsStyleContext::AssertStyleStructMaxDifferenceValid that was in part 9.

OK, I'll need to put those back.

Comment 127

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

Comment on attachment 8452893 [details] [diff] [review]
Part 17: Handle eRestyleResult_Stop by keeping the old style context and moving it to its new parent. (v2)

OK, now that I see the changes in patch 20, this seems ok.  (Though I'll have other comments on patch 20.)

>+    if (result == eRestyleResult_Stop) {
>+      nsStyleContext* newParent =
>+        mFrame->GetParentStyleContextFrame()->StyleContext();
>+
>+      if (oldContext->GetParent() == newParent) {
>+        // No need to move children across as we've got the same parent.
>+        return;
>+      }
>+
>+      // Keep the old style context on mFrame, but change it to point to
>+      // its new parent.
>+      oldContext->MoveTo(newParent);
>+
>+      // Send the accessibility notifications that RestyleChildren otherwise
>+      // would have sent.
>+      if (!(mHintsHandled & nsChangeHint_ReconstructFrame)) {
>+        InitializeAccessibilityNotifications();
>+        SendAccessibilityNotifications();
>+      }
>+
>+      return;

This would be clearer with only one "return" (the second one).  Instead of an early return (the first one), could you put the MoveTo inside the inverse of your current GetParent() check?  And, if you really meant it that way, put the accessibility notifications inside that check as well, although that feels like a mistake.

Comment 128

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

Comment on attachment 8452894 [details] [diff] [review]
Part 20: Stop the recursive restyling process when we detect it is safe to do so. (v2)

>+  // and we are consdiering stopping at this frame that also has a shared

s/consdiering/considering/


>+  // We ignore all situations that involve :visited style.
>+  if (oldContext->GetStyleIfVisited()) {
>+    return eRestyleResult_Continue;
>+  }
>+
>+  nsStyleContext* parentContext = oldContext->GetParent();
>+  if (parentContext && parentContext->GetStyleIfVisited()) {
>+    return eRestyleResult_Continue;
>+  }

I don't see why you need the second if here.

However, I think it would be a good idea to also check that there's
no GetStyleIfVisited on the new context in
ComputeRestyleResultFromNewContext, although I'm not sure that it's
necessary.

>+      uint32_t equalStructs;
>       TryStartingTransition(mPresContext, aSelf->GetContent(),
>                             oldContext, &newContext);
>-
>-      uint32_t equalStructs;
>       CaptureChange(oldContext, newContext, assumeDifferenceHint,
>                     &equalStructs);

Makes more sense to either (a) not make this move of equalStructs, or
(b) move the variable and the test of it outside of the if/else.

>+    if (result == eRestyleResult_Stop) {
>+      nsStyleContext* oldParent = oldContext->GetParent();
>+      do {
>+#define STYLE_STRUCT(name_, checkdata_cb)                                     \
>+        if (oldParent->HasCachedInheritedStyleData(eStyleStruct_##name_) &&   \
>+            !oldParent->HasSameCachedStyleData(parentContext,                 \
>+                                               eStyleStruct_##name_)) {       \
>+          result = eRestyleResult_Continue;                                   \
>+          break;                                                              \
>+        }
>+#include "nsStyleStructList.h"
>+#undef STYLE_STRUCT
>+      } while (false);
>     }

This does not require macros.  De-macroizing can be done by
transforming the above into the following:

    if (result == eRestyleResult_Stop) {
      nsStyleContext* oldParent = oldContext->GetParent();
      for (nsStyleStructID sid = nsStyleStructID(0);
           sid < nsStyleStructID_Length;
           sid = nsStyleStructID(sid + 1)) {
        if (oldParent->HasCachedInheritedStyleData(sid) &&
            !oldParent->HasSameCachedStyleData(parentContext, sid)) {
          result = eRestyleResult_Continue;
          break;
        }
      }
    }

Additionally, however, I don't see why you're doing these tests on
oldParent and parentContext.  I think they can be done on oldContext and
newContext.  (I don't think anything is broken either way, because I
think at this point if the tests fault to eRestyleResult_Continue on the
children (which at this point have to have the same rule node) then they
would also have to have faulted on the parent, but I think fixing this
would mean stopping the deoptimization one level of tree depth sooner
than we would with the current code, in cases where structs are
inherited.)

This case also requires a rather substantive comment.  I'd suggest:

    // Since we currently have eRestyleResult_Stop, we know at this
    // point that all of our style structs are equal in terms of styles.
    // However, some of them might be different pointers.  Since our
    // descendants might share those pointers, we have to continue to
    // restyling our descendants.
    //
    // However, because of the swapping of equal structs we've done on
    // ancestors in FIXME, we've ensured that for structs that cannot be
    // stored in the rule tree, we keep the old equal structs around
    // rather than replacing them with new ones.  This means that the
    // only time we hit this deoptimization is when at least one of the
    // (old or new) equal structs could be stored in the rule tree, and
    // those structs are then inherited (by pointer sharing) to
    // descendant style contexts.

Then you need to write a separate patch (plus resurrect patch 18) to
make the second paragraph of that comment true.


>-
>+  RestyleResult ComputeRestyleResultFromFrame(nsIFrame* aSelf);
>+  RestyleResult ComputeRestyleResultFromNewContext(nsIFrame* aSelf,
>+                                                   nsStyleContext* aNewContext);

Maybe leave the blank line below the insertion, instead of deleting it?


>+  bool HasSameDependentBits(nsStyleContext* aOther) {
>+    return (mBits & NS_STYLE_INHERIT_MASK) ==
>+           (aOther->mBits & NS_STYLE_INHERIT_MASK);
>+  }

Looks like this is unused and can be dropped.  (Or is it used in a
different patch?)


Anyway, r=dbaron with that, although I'm going to want to review the
additional patches that result from my comment above.

Comment 129

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

Comment on attachment 8413134 [details] [diff] [review]
Part 18: Add a function to swap style structs between style contexts

When you bring this back, please de-macro-ize, like in comment 128, using the loop forms as in nsInheritedStyleData constructor and nsResetStyleData constructor (in nsRuleNode.h).

Comment 131

[Cameron McCormack (:heycam)]

Attachment: Part 9: Make nsStyleContext::CalcStyleDifference compare all structs and return a bitfield of which changed. (no part 8) (v2)

Fixed a bug where we never returned the Variables struct bit in aEqualStructs.

Comment 132

[Cameron McCormack (:heycam)]

Attachment: Part 11: Add eRestyle_Force (and eRestyle_ForceDescendants) restyle hints to control whether the frame (and all of its descendants) must be assigned its new style context even if it had the ... (v2)

Added eRestyle_ForceDescendants so we can indicate "force the next level of style contexts to be recreated" versus "force the entire subtree of style context to be recreated".  This is needed for upcoming updated part 17.

Comment 133

[Cameron McCormack (:heycam)]

Attachment: Part 16: Give ElementRestyler::RestyleSelf a return value that indicates how restyling should proceed. (v2)

Renamed eRestyleResult_Continue to eRestyleResult_ContinueAndForceDescendants.

Comment 134

[Cameron McCormack (:heycam)]

Attachment: Part 17: Handle eRestyleResult_Stop by moving a frame's style context to its new parent. (v3)

We now handle continuations-with-same-style returning different RestyleResults.  (Turned out this case was triggered sometimes.)

Comment 135

[Cameron McCormack (:heycam)]

Attachment: Part 20: Stop the recursive restyling process when we detect it is safe to do so. (v3)

Comment 136

[Cameron McCormack (:heycam)]

Attachment: Part 20.1: Keep old structs on new style contexts, for those that are equal.

We don't do struct swapping if either the old or new style contexts are
shared, since we can't guarantee that such a change will be valid for
all frames that use the shared style context.

Comment 137

[Cameron McCormack (:heycam)]

Attachment: Part 20.2: Don't call AssertStructsNotUsedElsewhere on style contexts for :before/:after frames that are going away.

Seems a bit silly to reserve a bit just for DEBUG-only code; let me know if you have any better ideas.

Comment 138

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=66d49b784a3a

(The orange xpcshell tests are a problem with the inbound revision I was on top of.)

Comment 139

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

Since I was just figuring it out -- part 4 and part 8 both went away (comment 101 and comment 102) as a result of bug 1026344 and bug 1026345.

Comment 140

[Cameron McCormack (:heycam)]

Comment on attachment 8413128 [details] [diff] [review]
Part 12: Record on an ElementRestyler whether all ancestor frames we restyled had shared style contexts

Part 12 also can go, as the handling of shared style contexts is simpler now in part 20.

Comment 141

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

Comment on attachment 8463938 [details] [diff] [review]
Part 9: Make nsStyleContext::CalcStyleDifference compare all structs and return a bitfield of which changed. (no part 8) (v2)

From comment 80:
>In nsStyleContext.h, please document that CalcStyleDifference sets the
>equal bit when the old context didn't have the struct.

And could you file a followup bug on combining the branches from comment 
80 and the other followups mentioned there?

I also wonder if it's worth filing a followup on having an equality
check for nsStyleVariables that's more precise than pointer equality.
(Or is there a reason pointer equality is good enough?)

r=dbaron with that

Comment 142

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

Comment on attachment 8463939 [details] [diff] [review]
Part 11: Add eRestyle_Force (and eRestyle_ForceDescendants) restyle hints to control whether the frame (and all of its descendants) must be assigned its new style context even if it had the ... (v2)

>+ * Similarly, eRestyle_ForceDescendants will cause the frame and all of its
>+ * descendants to be traversed and for any new style contexts that are created
>+ * to be set on the frames.

The "any new style contexts" wording makes it sound like there might or might not be a new style context.  But doesn't this mean that there *will* be a new style context?  Or are there cases where it actually might stay the same?

r=dbaron with that fixed if needed

Comment 143

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #141)
> Comment on attachment 8463938 [details] [diff] [review]
>
> I also wonder if it's worth filing a followup on having an equality
> check for nsStyleVariables that's more precise than pointer equality.
> (Or is there a reason pointer equality is good enough?)

It's an operator== on the two CSSVariableValues structs.

Comment 144

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #142)
> Comment on attachment 8463939 [details] [diff] [review]
> Part 11: Add eRestyle_Force (and eRestyle_ForceDescendants) restyle hints to
> control whether the frame (and all of its descendants) must be assigned its
> new style context even if it had the ... (v2)
> 
> >+ * Similarly, eRestyle_ForceDescendants will cause the frame and all of its
> >+ * descendants to be traversed and for any new style contexts that are created
> >+ * to be set on the frames.
> 
> The "any new style contexts" wording makes it sound like there might or
> might not be a new style context.  But doesn't this mean that there *will*
> be a new style context?  Or are there cases where it actually might stay the
> same?

There are a couple of branches in ElementRestyler::RestyleSelf that deliberately leave the old style context on there (apart from the new optimisation), but I wasn't trying to draw attention to them really.  I'll do s/any/the/.

Comment 145

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

Comment on attachment 8463942 [details] [diff] [review]
Part 17: Handle eRestyleResult_Stop by moving a frame's style context to its new parent. (v3)

>+      nsStyleContext* newParent =
>+        mFrame->GetParentStyleContextFrame()->StyleContext();

I'm not crazy about adding new callers to GetParentStyleContextFrame.  It seems like it might be nicer if RestyleSelf passes it out as an out-parameter in the case where it returns eRestyleResult_Stop.  (I'm also ok postponing that to a followup, but I think it's quite easy.)

>+      if (oldContext->GetParent() != newParent) {

Is it possible for this check ever to fail?  It seems like they should always be different, though maybe I'm missing something.

r=dbaron

Comment 146

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

Comment on attachment 8463944 [details] [diff] [review]
Part 20: Stop the recursive restyling process when we detect it is safe to do so. (v3)

I'm still not sure why this bit is needed, although I'm also ok just 
landing with it:
>+  nsStyleContext* parentContext = oldContext->GetParent();
>+  if (parentContext && parentContext->GetStyleIfVisited()) {
>+    return eRestyleResult_Continue;
>+  }

r=dbaron

Comment 147

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

You should post the current Part 18 for review.

Comment 148

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #145)
> Comment on attachment 8463942 [details] [diff] [review]
> Part 17: Handle eRestyleResult_Stop by moving a frame's style context to its
> new parent. (v3)
...
> >+      if (oldContext->GetParent() != newParent) {
> 
> Is it possible for this check ever to fail?  It seems like they should
> always be different, though maybe I'm missing something.

I think this might happen in the :before/:after-frame-is-disappearing case, where we explicitly leave the old style context on the frame.  Its children then might return eRestyleResult_Stop.  But I will try a push with an assertion instead of the check to make sure.

Comment 149

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

Comment on attachment 8463947 [details] [diff] [review]
Part 20.1: Keep old structs on new style contexts, for those that are equal.

You should extend the big comment here:
>       // around rather than replacing them with new ones.  This means that the
>       // only time we hit this deoptimization is when at least one of the
>       // (old or new) equal structs could be stored in the rule tree, and
>       // those structs are then inherited (by pointer sharing) to
>       // descendant style contexts.
to mention that it also deoptimizes when the style contexts are shared
and we can't swap the equal structs.

r=dbaron with that

Comment 150

[Cameron McCormack (:heycam)]

Attachment: Part 18: Add a function to swap style structs between style contexts. (v3)

Comment 151

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

Comment on attachment 8463949 [details] [diff] [review]
Part 20.2: Don't call AssertStructsNotUsedElsewhere on style contexts for :before/:after frames that are going away.

I'm curious why this (and not any other reframe cases) are the only ones where we have a problem.  But not curious enough to block getting this landed.  Probably worth following up afterwards, though.

Comment 152

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

Comment on attachment 8463949 [details] [diff] [review]
Part 20.2: Don't call AssertStructsNotUsedElsewhere on style contexts for :before/:after frames that are going away.

Also perhaps add a comment in the bits list that says it's debug-only, so we know we can make it an #ifdef DEBUG member if we need to?

Comment 153

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

Comment on attachment 8464316 [details] [diff] [review]
Part 18: Add a function to swap style structs between style contexts. (v3)

>+   * Additionally, if there are identical struct pointers for one of the
>+   * structs indicated by aStructs, and it is not an owned struct on this,
>+   * then the cached struct slot on this will be set to null.

It would be nice to explain why, if you remember.  (I'm guessing it has something to do with if the struct was also swapped on the parent -- and maybe a relationship to making assertions not fire?)

r=dbaron

Comment 154

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #148)
> (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from
> comment #145)
> > Comment on attachment 8463942 [details] [diff] [review]
> > Part 17: Handle eRestyleResult_Stop by moving a frame's style context to its
> > new parent. (v3)
> ...
> > >+      if (oldContext->GetParent() != newParent) {
> > 
> > Is it possible for this check ever to fail?  It seems like they should
> > always be different, though maybe I'm missing something.
> 
> I think this might happen in the :before/:after-frame-is-disappearing case,
> where we explicitly leave the old style context on the frame.  Its children
> then might return eRestyleResult_Stop.  But I will try a push with an
> assertion instead of the check to make sure.

Plenty of failures when I try that.  https://tbpl.mozilla.org/?tree=Try&rev=7e9cfaedef32

Looking at just one of the failures (layout/reftests/reftest-sanity/filter-1.xhtml) we are restyling the root and keeping the old style context on there, which causes same style contexts to be used going down the tree.

Comment 155

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

Oh, for some reason I was thinking that was inside a Force or ForceDescendants case, but it wasn't.  Sorry.

Comment 156

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #153)
> Comment on attachment 8464316 [details] [diff] [review]
> Part 18: Add a function to swap style structs between style contexts. (v3)
> 
> >+   * Additionally, if there are identical struct pointers for one of the
> >+   * structs indicated by aStructs, and it is not an owned struct on this,
> >+   * then the cached struct slot on this will be set to null.
> 
> It would be nice to explain why, if you remember.  (I'm guessing it has
> something to do with if the struct was also swapped on the parent -- and
> maybe a relationship to making assertions not fire?)

OK, I worked it out again:

+   * Additionally, if there are identical struct pointers for one of the
+   * structs indicated by aStructs, and it is not an owned struct on this,
+   * then the cached struct slot on this will be set to null.  If the struct
+   * has been swapped on an ancestor, this style context (being the old one)
+   * will be left caching the struct pointer on the new ancestor, despite
+   * inheriting from the old ancestor.  This is not normally a problem, as
+   * this style context will usually be destroyed by being released at the
+   * end of ElementRestyler::Restyle; but for style contexts held on to outside
+   * of the frame, we need to clear out the cached pointer so that if we need
+   * it again we'll re-fetch it from the new ancestor.

Comment 157

[Cameron McCormack (:heycam)]

I rebased and pushed to try one more time and unfortunately there are a couple of test failures (the Gaia UI tests crash, and the test_additional_sheets.html timeout).

https://tbpl.mozilla.org/?tree=Try&rev=af501fddd4f2

Comment 158

[Cameron McCormack (:heycam)]

The test_additional_sheets.html timeout was due to the expensive assertions that I had enabled in that try run.

The Gaia UI test crash, and a crash I reproduced on a local Desktop B2G build, were due to an issue with struct swapping, for which I'll attach a patch in a moment.

There were also three tests that needed their expected assertions updating:

* test_animations_omta.html (bug 1012527), which now doesn't assert since we're tracking restyle roots to process after restyling a bit better.

* Intermittent oranges on test_bug572649.html and test_bug904810.html on B2G ICS Emulator, which are "wrong style context parent" assertions on scroll bar part frames.  As far as I can tell from looking at restyling logs, these don't result in anything dangerous, so I'm going to mark them as expected and file a followup to investigate them further.

I'll attach a patch for the assertion expectation updates too.

Comment 159

[Cameron McCormack (:heycam)]

Try run including the latest patches: https://tbpl.mozilla.org/?tree=Try&rev=da55cafad0d6

Oh and one more orange that showed up, after the Gaia UI tests crash was fixed, was a test failure that looks like bug 1032704, a fixed intermittent, but which has changed to be perma-orange.  I'm not sure what to do about that one; I've pushed a try run with a change to the fix in bug 1032704, increasing the timeout, to see if that's all that needs changing: https://tbpl.mozilla.org/?tree=Try&rev=db10e8d2f78a.  Not sure what to do about it if that doesn't work.

Comment 160

[Cameron McCormack (:heycam)]

Attachment: Part 22: Clear cached structs on descendants of an old style context that had structs swapped, if it is staying around.

We were performing struct swapping based on two assumptions, which turned out not to be true:

1. That if we do struct swapping on one style context, then the entire tree underneath it will get their structs swapped too, resulting in both the new style context subtree and the old one being valid.  But we end up not swapping structs in some situations, e.g. when the old or new style context is shared.

2. That the old style context for a frame (if we're not stopping restyling and leaving it on the frame) will no longer be held on to after we restyle the frame, and thus any struct swapping we do on that frame, even if it leaves the descendants with now-invalid cached structs, is OK since it'll be deleted by the end of ElementRestyler::Restyle.  This turned out not to be true, since e.g. an nsComputedDOMStyle object can hold on to the old style context.  There might be other things that hold on to the old style context too.

So we need to ensure that the old style context subtree is valid after we swap structs on its root.  We only need to do this if we did swap some structs, and if we know the style context is going to be kept alive beyond the ElementRestyler::Restyle call.  The latter I do by checking the refcount on the style context, which seems a bit hacky, but I think it's easiest way to check this.

Comment 161

[Cameron McCormack (:heycam)]

Attachment: Part 23: Change a few test assertion expectations.

I'll update the "Bug XXXXXXX" with real bug numbers once I file it.

Comment 162

[Cameron McCormack (:heycam)]

One more assertion seems to have popped up now in the try run:

###!!! ASSERTION: continuations should have the same style context: 'aOldStyleContext->GetPseudo() != nextStyle->GetPseudo() || aOldStyleContext->GetParent() != nextStyle->GetParent()', file ../../../gecko/layout/base/RestyleManager.cpp, line 1976

during test_transitions.html on B2G ICS Emulator Debug.

David, do you know off hand how dangerous this one would be if I just mark it expected?

Comment 163

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #159)
> Oh and one more orange that showed up, after the Gaia UI tests crash was
> fixed, was a test failure that looks like bug 1032704, a fixed intermittent,
> but which has changed to be perma-orange.  I'm not sure what to do about
> that one; I've pushed a try run with a change to the fix in bug 1032704,
> increasing the timeout, to see if that's all that needs changing:
> https://tbpl.mozilla.org/?tree=Try&rev=db10e8d2f78a.  Not sure what to do
> about it if that doesn't work.

OK, it didn't work.  Suggestions on what to do here would be welcome.

Comment 164

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #162)
> One more assertion seems to have popped up now in the try run:
> 
> ###!!! ASSERTION: continuations should have the same style context:
> 'aOldStyleContext->GetPseudo() != nextStyle->GetPseudo() ||
> aOldStyleContext->GetParent() != nextStyle->GetParent()', file
> ../../../gecko/layout/base/RestyleManager.cpp, line 1976
> 
> during test_transitions.html on B2G ICS Emulator Debug.

This is intermittent, btw.

Comment 165

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

(In reply to Cameron McCormack (:heycam) from comment #160)
> Created attachment 8466729 [details] [diff] [review]
> Part 22: Clear cached structs on descendants of an old style context that
> had structs swapped, if it is staying around.
> 
> We were performing struct swapping based on two assumptions, which turned
> out not to be true:
> 
> 1. That if we do struct swapping on one style context, then the entire tree
> underneath it will get their structs swapped too, resulting in both the new
> style context subtree and the old one being valid.  But we end up not
> swapping structs in some situations, e.g. when the old or new style context
> is shared.

But if it was safe to do struct swapping on the parent, doesn't that mean that it would also be safe to do struct swapping on the child even if it is shared, as long as we have a way to make sure it's only done once?  (I keep looking for a chunk of time to reason through this one more carefully; hopefully I'll have a chance to do that tomorrow.)


(In reply to Cameron McCormack (:heycam) from comment #162)
> One more assertion seems to have popped up now in the try run:
> 
> ###!!! ASSERTION: continuations should have the same style context:
> 'aOldStyleContext->GetPseudo() != nextStyle->GetPseudo() ||
> aOldStyleContext->GetParent() != nextStyle->GetParent()', file
> ../../../gecko/layout/base/RestyleManager.cpp, line 1976
> 
> during test_transitions.html on B2G ICS Emulator Debug.
> 
> David, do you know off hand how dangerous this one would be if I just mark
> it expected?

It seems like there's a decent chance it could end up associated with incorrect restyling, especially since we depend on continuations having the same style contexts in the restyling process.  Though maybe it's not that horrible...

Comment 166

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

Comment on attachment 8466729 [details] [diff] [review]
Part 22: Clear cached structs on descendants of an old style context that had structs swapped, if it is staying around.

I think we should be able to get rid of this in the long run, but let's do it for now so that we can get the rest of the patch series in, and then try to fix.


>+void
>+nsStyleContext::ClearCachedInheritedStyleDataOnDescendants(uint32_t aStructs)
>+{
>+  if (mChild) {
>+    nsStyleContext* child = mChild;
>+    do {
>+      child->DoClearCachedInheritedStyleDataOnDescendants(aStructs);
>+      child = child->mNextSibling;
>+    } while (mChild != child);
>+  }
>+  if (mEmptyChild) {
>+    nsStyleContext* child = mEmptyChild;
>+    do {
>+      child->DoClearCachedInheritedStyleDataOnDescendants(aStructs);
>+      child = child->mNextSibling;
>+    } while (mEmptyChild != child);
>+  }
>+}
>+
>+void
>+nsStyleContext::DoClearCachedInheritedStyleDataOnDescendants(uint32_t aStructs)
>+{
>+  for (nsStyleStructID i = nsStyleStructID_Inherited_Start;
>+       i < nsStyleStructID_Inherited_Start + nsStyleStructID_Inherited_Count;
>+       i = nsStyleStructID(i + 1)) {
>+    uint32_t bit = nsCachedStyleData::GetBitForSID(i);
>+    if (aStructs & bit) {
>+      if (!(mBits & bit) && mCachedInheritedData.mStyleStructs[i]) {
>+        aStructs &= ~bit;
>+      } else {
>+        mCachedInheritedData.mStyleStructs[i] = nullptr;
>+      }
>+    }
>+  }
>+
>+  if (mCachedResetData) {
>+    for (nsStyleStructID i = nsStyleStructID_Reset_Start;
>+         i < nsStyleStructID_Reset_Start + nsStyleStructID_Reset_Count;
>+         i = nsStyleStructID(i + 1)) {
>+      uint32_t bit = nsCachedStyleData::GetBitForSID(i);
>+      if (aStructs & bit) {
>+        if (!(mBits & bit) && mCachedResetData->mStyleStructs[i]) {
>+          aStructs &= ~bit;
>+        } else {
>+          mCachedResetData->mStyleStructs[i] = nullptr;
>+        }
>+      }
>+    }
>+  }
>+
>+  if (mChild) {
>+    nsStyleContext* child = mChild;
>+    do {
>+      child->DoClearCachedInheritedStyleDataOnDescendants(aStructs);
>+      child = child->mNextSibling;
>+    } while (mChild != child);
>+  }
>+  if (mEmptyChild) {
>+    nsStyleContext* child = mEmptyChild;
>+    do {
>+      child->DoClearCachedInheritedStyleDataOnDescendants(aStructs);
>+      child = child->mNextSibling;
>+    } while (mEmptyChild != child);
>+  }

This bit at the end looks the same as the fnuction above, so maybe you can just call it?

Also, you should bail somewhere when aStructs is 0, probably right before the identical bit, so that you don't have to traverse the whole tree.

>+  nsrefcnt RefCnt() const {
>+    return mRefCnt;
>+  }
>+

I'd prefer to make this an IsShared() that returns whether the reference count is 1 (and asserts it's not 0).

>+  // Helper for ClearCachedInheritedStyleDataOnDescendants.
>+  void DoClearCachedInheritedStyleDataOnDescendants(uint32_t aStructs);
>+

Maybe call this ClearCachedInheritedStyleDataOnSelfAndDescendants?

r=dbaron with that

Comment 167

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

Comment on attachment 8466730 [details]
Part 23: Change a few test assertion expectations.

ok, but we should dig into the "wrong parent style context" ones sooner rather than later

Comment 168

[Cameron McCormack (:heycam)]

Here's the Win8 crashtest failure: https://tbpl.mozilla.org/?tree=Try&rev=5eb4542cfa33

Comment 169

[Kai-Chih Hu [:khu]]

Hi, Milan, since you're the one to mark this bug with feature-b2g and you have better view on this bug from technical perspective, may I get your feedback to know if you're still targeting to land this to 2.1? Looks like the patch for this bug is big, and I was wondering, can we postpone this to 2.2? Thank you!

Comment 170

[Cameron McCormack (:heycam)]

Since this a relatively big change to how restyling works, I'd be uncomfortable landing this on not-trunk.

Made some progress on the Win8 crashtest failure today, FYI; image/test/crashtests/694165-1.xhtml loads a document with 300,000 <g> elements in a data: URL that hangs around (in the image cache?), which is the document we're restyling under the RebuildAllStyleData call (due to a pref change in a later test) when we're killed due to timeout.

Comment 171

[Milan Sreckovic [:milan] (needinfo for best results)]

This should definitely land on the trunk, when it's ready.  It is a performance improvement on its own, and also lets us make some other performance improvements, so if we can get it in 35, that'd be great.

Comment 172

[Cameron McCormack (:heycam)]

Attachment: Part 24: Avoid copying RestyleData::mDescendants when modifying an existing entry.

The Win8 crashtest timeout failure was due to an excessive amount of time being spent copying around RestyleData::mDescendants and AddRef/Releasing its contents.

This patch changes RestyleTracker::mPendingRestyles from an nsDataHashtable to an nsClassHashtable, which means that it stores a pointer to objects rather than the whole struct in its entry objects.  I split it up into RestyleHintData/RestyleData so that RestyleEnumerateData doesn't get the mDescendants array that it doesn't need.

Comment 173

[Cameron McCormack (:heycam)]

Try push showing the Win8 crashtest failure gone: https://tbpl.mozilla.org/?tree=Try&rev=20fb19a1a66c

Comment 174

[Cameron McCormack (:heycam)]

Final, green try run: https://tbpl.mozilla.org/?tree=Try&rev=2957519ecad9

Comment 175

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

Comment on attachment 8483298 [details] [diff] [review]
Part 24: Avoid copying RestyleData::mDescendants when modifying an existing entry.

CollectRestyles and CollectLaterSiblings should just take a
RestyleTracker::RestyleData* parameter since they don't mutate aData.

You don't seem to use the RestyleData/RestyleHintData distinction that you add, so why add it?  It seems better to just leave it as |struct RestyleData|.

r=dbaron with that

Comment 176

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #175)
> Comment on attachment 8483298 [details] [diff] [review]
> Part 24: Avoid copying RestyleData::mDescendants when modifying an existing
> entry.
> 
> CollectRestyles and CollectLaterSiblings should just take a
> RestyleTracker::RestyleData* parameter since they don't mutate aData.

That's not possible, since nsBaseHashtable::Enumerate expects a particular function prototype.  nsClassHashtable uses nsAutoPtr<UserDataType> as its DataType, and a reference to that is what's used in the EnumFunction argument type.

> You don't seem to use the RestyleData/RestyleHintData distinction that you
> add, so why add it?  It seems better to just leave it as |struct
> RestyleData|.

Oh, I did mean to use it, just to avoid the restylesToProcess array in RestyleTracker::DoProcessRestyles needing to use space for and initialize the mDescendants array that it doesn't use.  I'll leave it as is for the moment.

Comment 177

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #176)
> That's not possible, since nsBaseHashtable::Enumerate expects a particular
> function prototype.  nsClassHashtable uses nsAutoPtr<UserDataType> as its
> DataType, and a reference to that is what's used in the EnumFunction
> argument type.
> 
> > You don't seem to use the RestyleData/RestyleHintData distinction that you
> > add, so why add it?  It seems better to just leave it as |struct
> > RestyleData|.
> 
> Oh, I did mean to use it, just to avoid the restylesToProcess array in
> RestyleTracker::DoProcessRestyles needing to use space for and initialize
> the mDescendants array that it doesn't use.  I'll leave it as is for the
> moment.

That is, I meant to make RestyleEnumerateData inherit from RestyleHintData.

Comment 178

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

sounds good, but maybe file a followup on doing what you intended?

Comment 179

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66b4b0413366
https://hg.mozilla.org/mozilla-central/rev/6b664b975628
https://hg.mozilla.org/mozilla-central/rev/9f61117b493a
https://hg.mozilla.org/mozilla-central/rev/bae211382a0c
https://hg.mozilla.org/mozilla-central/rev/f502473815b3
https://hg.mozilla.org/mozilla-central/rev/effdf9950e60
https://hg.mozilla.org/mozilla-central/rev/f50e9c82b3b4
https://hg.mozilla.org/mozilla-central/rev/8fae88effe0d
https://hg.mozilla.org/mozilla-central/rev/579913aa3c8a
https://hg.mozilla.org/mozilla-central/rev/e11d2a79fe78
https://hg.mozilla.org/mozilla-central/rev/11244df7e9d9
https://hg.mozilla.org/mozilla-central/rev/ed3eb5c8ea76
https://hg.mozilla.org/mozilla-central/rev/d693bbec8d66
https://hg.mozilla.org/mozilla-central/rev/2c0731043053
https://hg.mozilla.org/mozilla-central/rev/4b2093c23781
https://hg.mozilla.org/mozilla-central/rev/90022d74c3d6
https://hg.mozilla.org/mozilla-central/rev/2c37ce863a3e
https://hg.mozilla.org/mozilla-central/rev/802abb82a0b3
https://hg.mozilla.org/mozilla-central/rev/8439b1a17178
https://hg.mozilla.org/mozilla-central/rev/52a7e3a19638
https://hg.mozilla.org/mozilla-central/rev/c6a6e10d2880
https://hg.mozilla.org/mozilla-central/rev/28f4f8564530
https://hg.mozilla.org/mozilla-central/rev/635055d16df6

Comment 180

[Jared Wein [:jaws] (please needinfo? me)]

Joel, did we see a CART win from this landing?

Comment 181

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for checking in on this- I see a 12.7% CART win for OSX 10.8 from push d934dc4a99ac:
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=d934dc4a99ac

This was right after another improvement for OSX 10.8 CART from push 4e7e1ce170ab:
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=4e7e1ce170ab

For the revision in question, I don't see any regressions or other wins, I assume this is what you were expecting?