1302054 - remove the CalcStyleDifference optimization handling the same-rule-node case

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

The optimization in CalcStyleDifference that avoids generating change hints if we know we have the same rule node and also know that we can't generate any changes due to reset property changes is no longer very effective now that we always compare all structs.  Once we do bug 1301258, and we more precisely cull changes that are subsumed by those already generated, it will have no use at all.  At that point we can remove it, some of the change hint state accumulated on ElementRestyler objects, and all of the MaxDifference / DifferenceAlwaysHandledForDescendants static methods on style structs.

Comment 1

[Cameron McCormack (:heycam)]

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1302054 - Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context.

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

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1302054 - Part 2: Remove no longer useful nsStyleContext::CalcDifference optimization that handles the same-rule-node case.

nsStyleContext::CalcDifference had an optimization where, when we knew
that the old and new style context have the same rule node, we knew that
the only change hints that would need to be handled are those in the
"not handled for descendants" category, generated due to explicit
'inherit' values on reset properties.  This was because any changes due
to differences in inherited properties should only have generated
"handled for descendants" change hints (and thus would already have been
handled on an ancestor).

Before bug 931668, this let us avoid calling CalcDifference on structs
that only would have generated hints that we knew we already would have
handled.  However, after bug 931668, we compare all structs anyway so
that we can set the aEqualStructs outparam, so we don't gain anything
from this optimization.  We can still return these change hints we know
will not need to be handled, and rely on ElementRestyler::CaptureChange
to filter them out.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1302054 - Part 3: Remove mention of nsChangeHint_Hints_NotHandledForDescendants in RestyleManager::ChangeHintToString.

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Bug 1302054 - Part 4: Make NeedDirtyReflow a not-always-handled-for-descendants change hint.

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

Comment 6

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

Comment on attachment 8794697 [details]
Bug 1302054 - Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context.

https://reviewboard.mozilla.org/r/81020/#review82664

::: layout/style/nsStyleContext.cpp:1346
(Diff revision 1)
>    return CalcStyleDifferenceInternal(&newContext, aParentHintsNotHandledForDescendants,
>                                       aEqualStructs, aSamePointerStructs);
>  }
>  
> +void
> +nsStyleContext::EnsureSameStructsCached(nsStyleContext* aOther)

I think aOldContext is a better name than aOther.

Comment 7

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

Comment on attachment 8794698 [details]
Bug 1302054 - Part 2: Remove no longer useful nsStyleContext::CalcDifference optimization that handles the same-rule-node case.

https://reviewboard.mozilla.org/r/81022/#review82666

::: layout/style/nsStyleContext.cpp:976
(Diff revision 1)
> -  // we worry about 'inherit' values.)
> -  bool compare = StyleSource() != aNewContext->StyleSource();
> -
>    DebugOnly<uint32_t> structsFound = 0;
>  
> -  // If we had any change in variable values, then we'll need to examine
> +  // XXXheycam: We should just do the comparison in

Probably prefer FIXME or FIXME(heycam) over XXXheycam.

::: layout/style/nsStyleContext.cpp:1016
(Diff revision 1)
> -  // In general, we want to examine structs starting with those that can
> -  // cause the largest style change, down to those that can cause the
> -  // smallest.  This lets us skip later ones if we already have a hint
> +  // The order of these DO_STRUCT_DIFFERENCE calls is no longer significant.
> +  // With a small amount of effort, we could replace them with a
> +  // #include "nsStyleStructList.h".

Add "FIXME" here.

Comment 8

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

Comment on attachment 8794699 [details]
Bug 1302054 - Part 3: Remove mention of nsChangeHint_Hints_NotHandledForDescendants in RestyleManager::ChangeHintToString.

https://reviewboard.mozilla.org/r/81024/#review82668

r=dbaron, although this might change once you address review comments in bug 1301258

::: layout/base/nsChangeHint.h
(Diff revision 1)
> -// The most hints that NS_HintsNotHandledForDescendantsIn could possibly return:
> -#define nsChangeHint_Hints_NotHandledForDescendants (      \
> -  nsChangeHint_Hints_NeverHandledForDescendants |          \
> -  nsChangeHint_ClearAncestorIntrinsics |                   \
> -  nsChangeHint_NeedReflow |                                \
> -  nsChangeHint_ReflowChangesSizeOrPosition                 \
> -)
> -

I think it would be useful to keep this as a "sometimes" category just so you can have an assertion that every change hint is in one of the categories (for the assertion I requested in bug 1301258).

Comment 9

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

Comment on attachment 8794700 [details]
Bug 1302054 - Part 4: Make NeedDirtyReflow a not-always-handled-for-descendants change hint.

https://reviewboard.mozilla.org/r/81018/#review82670

Hmmm.  I'm not sure about this one.  Why do you want it?

I guess it's because you added some invariant about the interaction between NeedDirtyReflow and ClearDescendantIntrinsics?

We really do want to keep as many hints as possible as handled for descendants.

(I guess I'm not entirely sure about the "old" (from bug 1301258) code in this patch either.)

Comment 10

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

Comment on attachment 8794700 [details]
Bug 1302054 - Part 4: Make NeedDirtyReflow a not-always-handled-for-descendants change hint.

https://reviewboard.mozilla.org/r/81018/#review96590

cancelling review for now; please re-request after answering comment 9

Comment 11

[Cameron McCormack (:heycam)]

Looks like we'll also be able to remove the aHintForThisFrame argument to the recently-added nsIFrame::UpdateStyleOfOwnedAnonBoxes, too.

Comment 12

[Cameron McCormack (:heycam)]

My current patches not longer make NeedDirtyReflow a not-always-handled-for-descendants hint.  I'm also deferring the removal of NS_HintsNotHandledForDescendantsIn to bug 1347828.

Comment 13

[Cameron McCormack (:heycam)]

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

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8794697 [details]
Bug 1302054 - Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context.

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

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8794698 [details]
Bug 1302054 - Part 2: Remove no longer useful nsStyleContext::CalcDifference optimization that handles the same-rule-node case.

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

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8794699 [details]
Bug 1302054 - Part 3: Remove mention of nsChangeHint_Hints_NotHandledForDescendants in RestyleManager::ChangeHintToString.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a196410f140
Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60081798b34
Part 2: Remove no longer useful nsStyleContext::CalcDifference optimization that handles the same-rule-node case. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7af7e53006
Part 3: Remove mention of nsChangeHint_Hints_NotHandledForDescendants in RestyleManager::ChangeHintToString. r=dbaron

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/6a196410f140
https://hg.mozilla.org/mozilla-central/rev/a60081798b34
https://hg.mozilla.org/mozilla-central/rev/bd7af7e53006