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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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 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.
Attachment #8794697 - Flags: review?(dbaron) → review+
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.
Attachment #8794698 - Flags: review?(dbaron) → review+
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).
Attachment #8794699 - Flags: review?(dbaron) → review+
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.)
Assignee: nobody → cam
Flags: needinfo?(cam)
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
Attachment #8794700 - Flags: review?(dbaron)
Looks like we'll also be able to remove the aHintForThisFrame argument to the recently-added nsIFrame::UpdateStyleOfOwnedAnonBoxes, too.
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.
Flags: needinfo?(cam)
Attachment #8794700 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
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
See Also: → 1351339
Depends on: 1353423
Depends on: 1376406
Depends on: 1364139
You need to log in before you can comment on or make changes to this bug.