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

(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.

(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

(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.

(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.

(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.

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.

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.

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

(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.

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.

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

(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?

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

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

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

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.

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.

(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.

(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.

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.

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

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.

(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.

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.

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.

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

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.

(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.)

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.

(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.

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

(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

(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).

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 ?

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.

(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.

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.

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.

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.

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

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

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.

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

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.

(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?

(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.

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

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 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 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

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

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 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 on attachment 8413121 [details] [diff] [review] Part 5: Record whether a style context is shared r=dbaron (out of bits for now!)

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 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 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 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 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 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 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 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 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 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 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 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.

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

(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.

(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?

(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.

(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?

(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.

(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.

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

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.

(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.

(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.

(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 on attachment 8413120 [details] [diff] [review] Part 4: Record whether a style context references objects allocated by its parent No longer needed.

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

(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.

(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.

(Question in comment 93.)

(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.)

(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.)

(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 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

ah, it needs s/which/with/

(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.

(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.

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 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.

(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 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

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 on attachment 8413134 [details] [diff] [review] Part 18: Add a function to swap style structs between style contexts No longer needed.

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.

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

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 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.)

(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.

(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 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 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 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).

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

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.

Renamed eRestyleResult_Continue to eRestyleResult_ContinueAndForceDescendants.

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

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.

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

https://tbpl.mozilla.org/?tree=Try&rev=66d49b784a3a (The orange xpcshell tests are a problem with the inbound revision I was on top of.)

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 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 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 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

(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.

(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 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 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

You should post the current Part 18 for review.

(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 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 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 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 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

(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.

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

(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.

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

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.

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.

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.

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

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?

(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.

(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.

(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 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 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