Closed Bug 1302054 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1302054 - Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context.
(deleted), text/x-review-board-request
dbaron
: review+
Details
Bug 1302054 - Part 2: Remove no longer useful nsStyleContext::CalcDifference optimization that handles the same-rule-node case.
(deleted), text/x-review-board-request
dbaron
: review+
Details
Bug 1302054 - Part 3: Remove mention of nsChangeHint_Hints_NotHandledForDescendants in RestyleManager::ChangeHintToString.
(deleted), text/x-review-board-request
dbaron
: review+
Details
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
Depends on: 1353423
Depends on: 1374752
Depends on: 1376406
Depends on: 1364139
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: