1301258 - mask off generated change hints for an element when only some have been subsumed by change hints on ancestors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

dbaron pointed out that in CaptureChange, we only decide not to append a change when all the changes we generated for this element have been subsumed by changes on ancestor.  Instead, we should just mask out the ones that were subsumed and append those.

Comment 1

[Cameron McCormack (:heycam)]

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

Comment 2

[Cameron McCormack (:heycam)]

Passing now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=baf416c58653

Talos didn't show any improvement, unfortunately, though I was able to construct a test case that runs faster with these changes.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=eac819013bf9&newProject=try&newRevision=8328e665512a&framework=1&showOnlyImportant=0

Will put patches up for review once dbaron returns.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: test

On my machine this test takes ~3500ms to restyle without the patches, and ~2200ms with the patches.

Comment 4

[Bobby Holley (:bholley)]

Shing, this would be a good testcase to put in your perfherder automation.

Comment 5

[Shing Lyu [:shinglyu]]

I'm adding a [stylo-perf-test] flag in the whiteboard for tracking.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

Review commit: https://reviewboard.mozilla.org/r/81012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81012/

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

Review commit: https://reviewboard.mozilla.org/r/81014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81014/

Comment 8

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

Comment on attachment 8794695 [details]
Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

https://reviewboard.mozilla.org/r/81012/#review81564

r=dbaron with the extra static_assert added (and maybe follow up on the other things?)

::: layout/base/nsChangeHint.h:297
(Diff revision 1)
> +#define nsChangeHint_Hints_AlwaysHandledForDescendants (   \
> +  nsChangeHint_ClearDescendantIntrinsics |                 \
> +  nsChangeHint_NeedDirtyReflow |                           \
> +  nsChangeHint_NeutralChange |                             \
> +  nsChangeHint_ReconstructFrame |                          \
> +  nsChangeHint_RepaintFrame |                              \
> +  nsChangeHint_SchedulePaint |                             \
> +  nsChangeHint_SyncFrameView |                             \
> +  nsChangeHint_UpdateCursor |                              \
> +  nsChangeHint_UpdateSubtreeOverflow |                     \
> +  nsChangeHint_UpdateTextPath                              \
> +)

Looking at this list, I wonder whether this is all actually true.  Perhaps we should audit the handling code for these hints and make sure it does what we think it does?

::: layout/base/nsChangeHint.h:317
(Diff revision 1)
> +  /* XXXheycam: NS_HintsNotHandledForDescendantsIn doesn't try to */ \
> +  /* optimize UpdateComputedBSize, but NS_RemoveSubsumedHints     */ \
> +  /* does; we'll remove it from here in a later patch, when we    */ \
> +  /* remove NS_HintsNotHandledForDescendantsIn.                   */ \

I don't completely follow this, but I guess I'll worry about that later when you change it.

::: layout/base/nsChangeHint.h:340
(Diff revision 1)
> -          nsChangeHint_UpdateParentOverflow | \
> -          nsChangeHint_ChildrenOnlyTransform | \
> -          nsChangeHint_RecomputePosition | \
> -          nsChangeHint_UpdateContainingBlock | \
> -          nsChangeHint_BorderStyleNoneChange | \
> +static_assert(!(nsChangeHint_Hints_AlwaysHandledForDescendants &
> +                nsChangeHint_Hints_NeverHandledForDescendants),
> +              "change hints must not be present in both "
> +              "nsChangeHint_Hints_AlwaysHandledForDescendants and "
> +              "nsChangeHint_Hints_NeverHandledForDescendants");

Could you also static_assert that
nsChangeHint_Hints_NotHandledForDescendants | nsChangeHint_Hints_AlwaysHandledForDescentants == nsChangeHint_AllHints?

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8794695 [details]
Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

https://reviewboard.mozilla.org/r/81012/#review81564

> I don't completely follow this, but I guess I'll worry about that later when you change it.

Oh, I referenced a function from a future patch there.

> Could you also static_assert that
> nsChangeHint_Hints_NotHandledForDescendants | nsChangeHint_Hints_AlwaysHandledForDescentants == nsChangeHint_AllHints?

This condition isn't true, specifically there are those hints that can't be considered as "never handled for descendants" or "always handled for descendants" (like the current NeedReflow, ReflowChangesSizeOrPosition and ClearAncestorIntrinsics bits) which get special handling in the future patch's NS_RemoveSubsumedHints.

Comment 10

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

(In reply to Cameron McCormack (:heycam) from comment #9)
> > Could you also static_assert that
> > nsChangeHint_Hints_NotHandledForDescendants | nsChangeHint_Hints_AlwaysHandledForDescentants == nsChangeHint_AllHints?
> 
> This condition isn't true, specifically there are those hints that can't be
> considered as "never handled for descendants" or "always handled for
> descendants" (like the current NeedReflow, ReflowChangesSizeOrPosition and
> ClearAncestorIntrinsics bits) which get special handling in the future
> patch's NS_RemoveSubsumedHints.

But those are the three hints that are in nsChangeHint_Hints_NotHandledForDescendants in addition to the ones in NeverHandledForDescendants.  So I still think you should be able to assert this.

Comment 11

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

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

https://reviewboard.mozilla.org/r/81014/#review82572

This is a somewhat tentitive review-.  (I'd leave it review?, except that wouldn't generate bugzilla request mail so you might miss it!)  See my comments below for why.

::: layout/base/RestyleManager.cpp:1386
(Diff revision 1)
>      // XXXldb Why does it make sense to use aParentContent?  (See
>      // comment above assertion at start of ElementRestyler::Restyle.)
>    , mContent(mFrame->GetContent() ? mFrame->GetContent() : mParentContent)
>    , mChangeList(aChangeList)
>    , mHintsHandled(aHintsHandledByAncestors &
> -                  ~NS_HintsNotHandledForDescendantsIn(aHintsHandledByAncestors))
> +                  ~nsChangeHint_Hints_NeverHandledForDescendants)

So these constructor changes (and the changes later that they're tied
to) mean that
what ElementRestyler::mHintsHandled means has changed a bit, since the
way handled bits from ancestors are propagated to it has changed.  In
particular, it will now contain nsChangeHint_NeedReflow,
nsChangeHint_ReflowChangesSizeOrPosition, or
nsChangeHint_ClearAncestorIntrinsics in the cases where
NS_HintsNotHandledForDescendantsIn would have concluded those hints were
handled for descendants.  It's not yet clear to me why you've made this
change.  But I'll come back to this below...

::: layout/base/RestyleManager.cpp:1622
(Diff revision 1)
> -    mHintsHandled |= ourChange;
> +  nsChangeHint changeToAppend =
> +    NS_RemoveSubsumedHints(mHintsHandled, ourChange);
> +
> +  if (changeToAppend) {
> +    mHintsHandled |= changeToAppend;

So I think there's a substantive problem here.  We currently, since we haven't done bug 918064 yet, call RestyleSelf for each continuation (and ib-split sibling).  This test was what prevented us from posting the same change hint for every continuation.

Now that you're assuming mHintsHandled needs to be treated pessimistically as though it came from the parent (see my comment above, on the constructor), this means that certain hints will result in changes being posted over again for every continuation.

Is there a way this can work with the old structure, where the pessimism about what could be transferred from parent to child could continue to be handled in the constructor?  That would avoid this problem.  (Is there an advantage to this structure?)

::: layout/base/RestyleManager.cpp:1632
(Diff revision 1)
>      if (!(ourChange & nsChangeHint_ReconstructFrame) || mContent) {
>        LOG_RESTYLE("appending change %s",
> -                  RestyleManager::ChangeHintToString(ourChange).get());
> -      mChangeList->AppendChange(mFrame, mContent, ourChange);
> +                  RestyleManager::ChangeHintToString(changeToAppend).get());
> +      mChangeList->AppendChange(mFrame, mContent, changeToAppend);
>      } else {
> -      LOG_RESTYLE("change has already been handled");
> +      LOG_RESTYLE("ignoring ReconstructFrame change with no content");

The message for this used to be "change has already been handled".  Maybe the author of that text knew where else it was, and the new text should still say that?

::: layout/base/nsChangeHint.h:406
(Diff revision 1)
> +inline nsChangeHint
> +NS_RemoveSubsumedHints(nsChangeHint aHintsHandled, nsChangeHint aOurChange)

I tend to think the parameters to this function should be in the opposite order, since it works rather like subtraction, which makes me expect the parameters to be in the order they'd be in for subtraction.

I also haven't reviewed the body of this function carefully yet given my comments above.

Comment 12

[Bobby Holley (:bholley)]

See bug 1340022 comment 6 for the servo optimization that we'll unlock if we can get this landed.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8794695 [details]
Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

https://reviewboard.mozilla.org/r/81012/#review81564

> Looking at this list, I wonder whether this is all actually true.  Perhaps we should audit the handling code for these hints and make sure it does what we think it does?

That's probably a good idea.

> Oh, I referenced a function from a future patch there.

It's probably not worth that comment being there anyway; I'll just remove it.

> This condition isn't true, specifically there are those hints that can't be considered as "never handled for descendants" or "always handled for descendants" (like the current NeedReflow, ReflowChangesSizeOrPosition and ClearAncestorIntrinsics bits) which get special handling in the future patch's NS_RemoveSubsumedHints.

Looking back at this now, I think I misread your comment saying to assert `(NeverHandled | AlwaysHandled) == AllHints`, but actually you asked for `(NotHandled | AlwaysHandled) == AllHints`.

In any case, I will split the bits out into three separate #defines (for "Always", "Never" and "Sometimes"), to make it a bit clearer.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

https://reviewboard.mozilla.org/r/81014/#review82572

> So these constructor changes (and the changes later that they're tied
> to) mean that
> what ElementRestyler::mHintsHandled means has changed a bit, since the
> way handled bits from ancestors are propagated to it has changed.  In
> particular, it will now contain nsChangeHint_NeedReflow,
> nsChangeHint_ReflowChangesSizeOrPosition, or
> nsChangeHint_ClearAncestorIntrinsics in the cases where
> NS_HintsNotHandledForDescendantsIn would have concluded those hints were
> handled for descendants.  It's not yet clear to me why you've made this
> change.  But I'll come back to this below...

Ultimately, the optimization all of this works towards is "can we avoid generating hints if we know they're already handled".  So that lead me to believe that using a function that removed the hints which we found were already handled (NS_RemoveSubsumedHints), at the point where we've just called CalcStyleDifference, would be clearer than a function that did the opposite (removed hints that are handled for descendants) and then using its negation.

It's a good point about more bits being left in mHintsHandled.  That seems wrong.

I am wondering now, especially given your comments about the same-style-continuations, whether we should separately store "hints handled by ancestors" and "hints handled so far for this element" on ElementRestyler.  Let's say we rename mHintsHandled to mHintsHandledByAncestors, and we initialize it just with aHintsHandledByAncestors (for the top level constructor) or with (aParentRestyler.mHintsHandledByAncestors | aParentRestyler.mHintsHandledBySelf) for the child restyler case.  In both cases we don't mask anything out.  Then we add a new mHintsHandledBySelf, which starts off empty, and then for each CaptureChange call when we generate a new change hint from CalcStyleDifference, we (1) call NS_RemoveSubsumedHints to remove mHintsHandledByAncestors from it, and (2) check if this new value is equal to mHintsHandledBySelf, and skip appending it if it's exactly equal [preserving the current handling of same-style-sibling hint handling].

That sounds like it should preserve the current level of pessimism about which hints were indeed handle by ancestors (for those that may or may not be).  And it should let us continue to use NS_RemoveSubsumedHints, which I think is an easier concept to understand.

Does this sound OK?

> So I think there's a substantive problem here.  We currently, since we haven't done bug 918064 yet, call RestyleSelf for each continuation (and ib-split sibling).  This test was what prevented us from posting the same change hint for every continuation.
> 
> Now that you're assuming mHintsHandled needs to be treated pessimistically as though it came from the parent (see my comment above, on the constructor), this means that certain hints will result in changes being posted over again for every continuation.
> 
> Is there a way this can work with the old structure, where the pessimism about what could be transferred from parent to child could continue to be handled in the constructor?  That would avoid this problem.  (Is there an advantage to this structure?)

Thanks for pointing out the real reason behind the equality check in CaptureChange, which wasn't at all obvious to me.

> The message for this used to be "change has already been handled".  Maybe the author of that text knew where else it was, and the new text should still say that?

The author of that text (me) just originally added the LOG_RESTYLE to the wrong if statement's else branch.  This fixes it.

Comment 15

[Cameron McCormack (:heycam)]

Sorry for the substantial lag in replying to your review comments, David.  Let me know if you think comment 14 makes sense and I'll update the patch if so.

FWIW I don't really understand when it would or wouldn't make sense to optimize further the handling of non-identical change hints generated on later same-style-continuations.  It seems like whether a given hint gets applied to later sibling continuations depends on the hint, so it might not be simple to do something smarter.  (And maybe we should just do bug 918064 instead of worrying about that, although as I just commented in that bug it may or may not be straightforward to do.)

Comment 16

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

(In reply to Cameron McCormack (:heycam) from comment #13)
> In any case, I will split the bits out into three separate #defines (for
> "Always", "Never" and "Sometimes"), to make it a bit clearer.

Sounds great.

(In reply to Cameron McCormack (:heycam) from comment #14)
> I am wondering now, especially given your comments about the
> same-style-continuations, whether we should separately store "hints handled
> by ancestors" and "hints handled so far for this element" on
> ElementRestyler.  Let's say we rename mHintsHandled to
> mHintsHandledByAncestors, and we initialize it just with
> aHintsHandledByAncestors (for the top level constructor) or with
> (aParentRestyler.mHintsHandledByAncestors |
> aParentRestyler.mHintsHandledBySelf) for the child restyler case.  In both
> cases we don't mask anything out.  Then we add a new mHintsHandledBySelf,
> which starts off empty, and then for each CaptureChange call when we
> generate a new change hint from CalcStyleDifference, we (1) call
> NS_RemoveSubsumedHints to remove mHintsHandledByAncestors from it, and (2)
> check if this new value is equal to mHintsHandledBySelf, and skip appending
> it if it's exactly equal [preserving the current handling of
> same-style-sibling hint handling].
> 
> That sounds like it should preserve the current level of pessimism about
> which hints were indeed handle by ancestors (for those that may or may not
> be).  And it should let us continue to use NS_RemoveSubsumedHints, which I
> think is an easier concept to understand.
> 
> Does this sound OK?

I think it does (at least assuming NS_RemoveSubsumedHints preserves the behavior of NS_HintsNotHandledForDescendantsIn, which I didn't double-check), but it's definitely worth actually testing that the right things happen for the reflow hints and that we don't start accumulating them on descendants as a result of the revised patch.

Although I'm still not sure I see what the advantage is of keeping around the original bits from the ancestors.

Comment 17

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

(In reply to Cameron McCormack (:heycam) from comment #15)
> FWIW I don't really understand when it would or wouldn't make sense to
> optimize further the handling of non-identical change hints generated on
> later same-style-continuations.  It seems like whether a given hint gets
> applied to later sibling continuations depends on the hint, so it might not
> be simple to do something smarter.  (And maybe we should just do bug 918064
> instead of worrying about that, although as I just commented in that bug it
> may or may not be straightforward to do.)

I believe that all hints apply to all continuations (and all block-in-inline-siblings), and any hint handling code that doesn't deal with that is buggy.  I think the idea of a hint that applies to only some continuations is rare enough that it's not worth optimizing.

But I think there are some obscure cases where we might generate a hint for a later continuation that wasn't generated for earlier continuations (e.g., if part of an element is in a ::first-line, and coincident style changes mean that only the part outside of the first line changes style).

I think it's still worth doing bug 918064.

Comment 18

[Cameron McCormack (:heycam)]

(This is a Gecko optimization, but one whose code opens the way for a similar Stylo optimization.)

Comment 19

[Cameron McCormack (:heycam)]

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

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Bug 1301258 - Part 1.5: Tweak some comments around change hints.

Review commit: https://reviewboard.mozilla.org/r/120958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120958/

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8794695 [details]
Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81012/diff/1-2/

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81014/diff/1-2/

Comment 23

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

Comment on attachment 8848001 [details]
Bug 1301258 - Part 1.5: Tweak some comments around change hints.

https://reviewboard.mozilla.org/r/120958/#review123156

Comment 24

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

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

https://reviewboard.mozilla.org/r/81014/#review123158

I think a bunch of the handling of reflow hints is pretty wrong, and makes the optimizations substantially weaker than what we have pre-patch, probably resulting in a flood of hints in a number of cases.

Other than that, the patch looks good, though, but I need to re-review the reflow hints stuff.

::: layout/base/nsChangeHint.h:189
(Diff revision 2)
> -   * parent.  Must be not be set without also setting nsChangeHint_NeedReflow.
> +   * parent.  Must be not be set without also setting nsChangeHint_NeedReflow
> +   * and nsChangeHint_ClearAncestorIntrinsics.

maybe this should move to patch 1.5?  (looks fine, though)

::: layout/base/nsChangeHint.h:438
(Diff revision 2)
> +  // XXXheycam NS_RemoveSubsumedHints treats ReflowChangesSizeOrPosition as
> +  // always handled for descendants, but NS_HintsNotHandledForDescendantsIn
> +  // only sometimes removes it.  Once we remove
> +  // NS_HintsNotHandledForDescendantsIn, we can move this hint up to
> +  // nsChangeHint_Hints_AlwaysHandledForDescendants.
> +  result &= ~(aHintsHandled & nsChangeHint_ReflowChangesSizeOrPosition);

Both the code and the comment here are wrong.

ReflowChangesSizeOrPosition (which exists to figure out what the right behavior is for reflow roots, and is essentially a modifier for NeedReflow) can only be removed if aHintsHandled includes NeedDirtyReflow.  If aHintsHandled doesn't include NeedDirtyReflow, then we need to keep ReflowChangesSizeOrPosition, just like we need to keep NeedReflow.

::: layout/base/nsChangeHint.h:448
(Diff revision 2)
> +    // this frame.  However, if our change has ReflowChangesSizeOrPosition and
> +    // our handled change doesn't, then we need to preserve
> +    // ClearAncestorIntrinsics as it can end up marking more intrinsic ISizes
> +    // dirty up the tree.  This is because ReflowChangesSizeOrPosition causes

This doesn't make any sense.  There's no need to preserve ClearAncestorIntrinsics if an ancestor did ClearDescendantIntrinsics.

::: layout/base/nsChangeHint.h:457
(Diff revision 2)
> +    // reflow-related hints.  So if we ever generated (NeedReflow |
> +    // ReflowChangesSizeOrPosition) on a single ancestor, but without any of
> +    // the other reflow-related hints, then we won't actually have done any
> +    // MarkIntrinsicISizeDirty calls up the tree, and so this test below would

I don't think this makes sense either.

A way to think about ReflowChangesSizeOrPosition is that it's sort of like a single-parent promotion of the hint.  In other words, NeedReflow | ReflowChangesSizeOrPosition on an element sort of implies NeedReflow on the parent.

In other words, I think the test just below this comment that checks (aHintsHandled & nsChangeHint_ReflowChangesSizeOrPosition) before clearing the ClearAncestorIntrinsics bit is unneeded; you should just be able to clear the ClearAncestorIntrinsics bit if an ancestor had ClearDescendantIntrinsics, without the test.

::: layout/base/nsChangeHint.h:477
(Diff revision 2)
> +    // A ClearDescendantIntrinsics on an ancestor will also cause
> +    // HAS_DIRTY_CHILDREN to be set on this frame.  So if there are no other
> +    // reflow hints that need it, we can remove NeedReflow.
> +    if (!(result & (nsChangeHint_ClearAncestorIntrinsics |
> +                    nsChangeHint_ClearDescendantIntrinsics |
> +                    nsChangeHint_ReflowChangesSizeOrPosition |
> +                    nsChangeHint_NeedDirtyReflow))) {
> +      result &= ~nsChangeHint_NeedReflow;
> +    }

This is not aggressive enough, and I think will result in a lot of duplicated hints.  It's not clear to me why you're making this so much weaker an optimization than what NS_HintsNotHandledForDescendantsIn does.  (And I'm not sure why you've tried to merge the handling of NeedReflow / NeedDirtyReflow / ReflowChangesSizeOrPosition and the handling of ClearDescendantIntrinsics / ClearAncestorIntrinsics, which are separate there, and which I believe can stay separate.)

In particular, if an ancestor did ClearDescendantIntrinsics and NeedDirtyReflow, we can remove all of ClearDescendantIntrinsics, ClearAncestorIntrinsics, NeedDirtyReflow, NeedReflow, ReflowChangesSizeOrPosition, and UpdateComputedBSize.  (We didn't historically do this for UpdateComputedBSize, but we can.  It would go in the ClearDescendantIntrinsics/ClearAncestorIntrinsics half of the logic, and be subsumed by a ClearDescendantIntrinsics on an ancestor.)

::: layout/base/nsChangeHint.h:486
(Diff revision 2)
> +  } else if (aHintsHandled & nsChangeHint_NeedDirtyReflow) {
> +    // If we marked an ancestor as NS_FRAME_IS_DIRTY, then there is no need
> +    // to mark any descendant as NS_FRAME_HAS_DIRTY_CHILDREN, since we'll
> +    // end up reflowing it anyway.  So if there are no other reflow
> +    // hints that need it, we can remove NeedReflow.
> +    if (!(result & (nsChangeHint_ClearAncestorIntrinsics |
> +                    nsChangeHint_ClearDescendantIntrinsics |
> +                    nsChangeHint_ReflowChangesSizeOrPosition |
> +                    nsChangeHint_NeedDirtyReflow))) {
> +      result &= ~nsChangeHint_NeedReflow;
> +    }

This is also a weaker optimization than we used to have, e.g., by not removing NeedReflow *and* ReflowChangesSizeOrPosition when NeedDirtyReflow was set on an ancestor.

Comment 25

[Cameron McCormack (:heycam)]

OK, I think in the interests of getting the main optimization in (the ElementRestyler bits of part 2), I'm going to defer the NS_RemoveSubsumedHints bits for now, and come back to it when I have some more time to work through it.

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8794695 [details]
Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

https://reviewboard.mozilla.org/r/81012/#review81564

> That's probably a good idea.

Filed bug 1348624.

Comment 27

[Cameron McCormack (:heycam)]

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

https://reviewboard.mozilla.org/r/81014/#review123158

> This doesn't make any sense.  There's no need to preserve ClearAncestorIntrinsics if an ancestor did ClearDescendantIntrinsics.

Are you sure?  I thought that the presence of ReflowChangesSizeOrPosition can cause us to iterate up the tree to the second-closest reflow root (and when not present it goes up to the closest reflow root).  So if an ancestor generated ClearDescendantIntrinsics (without ReflowChangesSizeOrPosition), and then the current node generated ClearAncestorIntrinsics|ReflowChangesSizeOrPosition, then we do need to append that change hint, since it would do some work that the ancestor's change hints wouldn't do -- i.e., mark HAS_DIRTY_CHILDREN and call MarkIntrinsicISizeDirty on nodes between the closest and second closest reflow root.

Am I misreading what PresShell::FrameNeedsReflow does?

Comment 28

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

(In reply to Cameron McCormack (:heycam) from comment #27)
> Are you sure?  I thought that the presence of ReflowChangesSizeOrPosition
> can cause us to iterate up the tree to the second-closest reflow root (and
> when not present it goes up to the closest reflow root).

But the distinction should be only when the target of the change hint is itself a reflow root.  (Perhaps that's not clear from the comments?  Or is the code wrong?)

> So if an ancestor
> generated ClearDescendantIntrinsics (without ReflowChangesSizeOrPosition),
> and then the current node generated
> ClearAncestorIntrinsics|ReflowChangesSizeOrPosition, then we do need to
> append that change hint, since it would do some work that the ancestor's
> change hints wouldn't do -- i.e., mark HAS_DIRTY_CHILDREN and call
> MarkIntrinsicISizeDirty on nodes between the closest and second closest
> reflow root.
> 
> Am I misreading what PresShell::FrameNeedsReflow does?

I think you are.  aRootHandling is used *only* to compute targetNeedsReflowFromParent.  targetNeedsReflowFromParent is used *only in a test of:
   (_f != subtreeRoot || !targetNeedsReflowFromParent)
where subtreeRoot (excluding the bit about traversing out-of-flows) is the frame that we requested to reflow (aFrame) in the first place.

It's possible the interaction of enqueueing out-of-flows with targetNeedsReflowFromParent might be incorrect, although I suspect it's actually right as it is.

Comment 29

[Cameron McCormack (:heycam)]

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

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8794695 [details]
Bug 1301258 - Part 1: Define macros for change hints always, never and sometimes handled for descendants.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81012/diff/2-3/

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8848001 [details]
Bug 1301258 - Part 1.5: Tweak some comments around change hints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120958/diff/1-2/

Comment 32

[Cameron McCormack (:heycam)]

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81014/diff/2-3/

Comment 33

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

Comment on attachment 8794696 [details]
Bug 1301258 - Part 2: Remove all subsumed hints when generating changes from restyles.

https://reviewboard.mozilla.org/r/81014/#review124116

r=dbaron with the following fixed

::: layout/base/GeckoRestyleManager.cpp:1346
(Diff revision 3)
> +  // NS_RemoveSubsumedHints, but aimed at removing hints handled
> +  // only for the current element instead.  However, we should probably just
> +  // fix these rare cases as part of bug 918064.)

The wrapping of these 3 lines became a bit odd (too short, compared to lines above) with this revision of the patch.

::: layout/base/GeckoRestyleManager.cpp:1832
(Diff revision 3)
>      mContent->OwnerDoc()->FlushPendingLinkUpdates();
>      nsAutoPtr<RestyleTracker::RestyleData> restyleData;
>      if (mRestyleTracker.GetRestyleData(mContent->AsElement(), restyleData)) {
> -      if (!NS_IsHintSubset(restyleData->mChangeHint, mHintsHandled)) {
> -        mHintsHandled |= restyleData->mChangeHint;
> -        mChangeList->AppendChange(mFrame, mContent, restyleData->mChangeHint);
> +      nsChangeHint changeToAppend =
> +        NS_RemoveSubsumedHints(restyleData->mChangeHint,
> +	                       mHintsHandledByAncestors);

This line now has a tab on it (although I only see it when actually diffing the hg revisions; MozReview doesn't show it).

This was introduced since the last review.

::: layout/base/nsChangeHint.h:448
(Diff revision 3)
> +  if (result & (nsChangeHint_ClearDescendantIntrinsics |
> +                nsChangeHint_ReflowChangesSizeOrPosition)) {
> +    result |= nsChangeHint_ClearAncestorIntrinsics |
> +              nsChangeHint_NeedDirtyReflow;
> +  }

This still seems wrong.

In particular, returning ReflowChangesSizeOrPosition without returning NeedDirtyReflow is fine.  ('margin', 'width', and 'height' changes do that, for example.)

If NS_HintsHandledForDescendantsIn removed NeedDirtyReflow, then I think it's fine to leave it removed, unless we require it because of assertions related to ClearDescendantIntrinsics.

So I think you should remove ReflowChangesSizeOrPosition from this test.

It's also not clear to me how we could end up in a situation with ClearDescendantIntrinsics present but ClearAncestorIntrinsics absent, since NS_HintsNotHandledForDescendantsIn(aHintsHandled) will only contain ClearAncestorIntrinsics if aHintsHandled doesn't contain ClearDescendantIntrinsics.  Combining that with the invariant that we never report ClearDescendantIntrinsics without also reporting ClearAncestorIntrinsics should mean that the result of NS_HintsHandledForDescendantsIn (note no "Not") always contains both bits (AncestorIntrinsics & DescendantIntrinsics) or neither.  So I think the ClearAncestorIntrinsics line can also be removed.  But I'm a little less sure about this bit.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22e3e7b04790
Part 1: Define macros for change hints always, never and sometimes handled for descendants. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa7dc151849
Part 1.5: Tweak some comments around change hints. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/70914315e02b
Part 2: Remove all subsumed hints when generating changes from restyles. r=dbaron

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22e3e7b04790
https://hg.mozilla.org/mozilla-central/rev/3fa7dc151849
https://hg.mozilla.org/mozilla-central/rev/70914315e02b