Skip to content

Commit

Permalink
Bug 1302054 - Part 2: Remove no longer useful nsStyleContext::CalcDif…
Browse files Browse the repository at this point in the history 
…ference optimization that handles the same-rule-node case. r=dbaron

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.

MozReview-Commit-ID: Ld1s2Js0i6r
  • Loading branch information
@heycam
heycam committed Mar 21, 2017
1 parent 5131fe2 commit cf56de7
Show file tree
Hide file tree
Showing 10 changed files with 9 additions and 401 deletions.
1 change: 0 additions & 1 deletion dom/animation/KeyframeEffectReadOnly.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1670,7 +1670,6 @@ KeyframeEffectReadOnly::CalculateCumulativeChangeHint(
uint32_t samePointerStructs = 0;
nsChangeHint changeHint =
fromContext->CalcStyleDifference(toContext,
nsChangeHint(0),
&equalStructs,
&samePointerStructs);

Expand Down
25 changes: 1 addition & 24 deletions layout/base/GeckoRestyleManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1061,8 +1061,6 @@ ElementRestyler::ElementRestyler(nsPresContext* aPresContext,
, mChangeList(aChangeList)
, mHintsHandledByAncestors(aHintsHandledByAncestors)
, mHintsHandledBySelf(nsChangeHint(0))
, mParentFrameHintsNotHandledForDescendants(nsChangeHint(0))
, mHintsNotHandledForDescendants(nsChangeHint(0))
, mRestyleTracker(aRestyleTracker)
, mSelectorsForDescendants(aSelectorsForDescendants)
, mTreeMatchContext(aTreeMatchContext)
Expand Down Expand Up @@ -1112,9 +1110,6 @@ ElementRestyler::ElementRestyler(const ElementRestyler& aParentRestyler,
((aConstructorFlags & FOR_OUT_OF_FLOW_CHILD) ?
~nsChangeHint_AllReflowHints : ~nsChangeHint(0)))
, mHintsHandledBySelf(nsChangeHint(0))
, mParentFrameHintsNotHandledForDescendants(
aParentRestyler.mHintsNotHandledForDescendants)
, mHintsNotHandledForDescendants(nsChangeHint(0))
, mRestyleTracker(aParentRestyler.mRestyleTracker)
, mSelectorsForDescendants(aParentRestyler.mSelectorsForDescendants)
, mTreeMatchContext(aParentRestyler.mTreeMatchContext)
Expand Down Expand Up @@ -1151,10 +1146,6 @@ ElementRestyler::ElementRestyler(ParentContextFromChildFrame,
, mHintsHandledByAncestors(aParentRestyler.mHintsHandledByAncestors |
aParentRestyler.mHintsHandledBySelf)
, mHintsHandledBySelf(nsChangeHint(0))
, mParentFrameHintsNotHandledForDescendants(
// assume the worst
nsChangeHint_Hints_NotHandledForDescendants)
, mHintsNotHandledForDescendants(nsChangeHint(0))
, mRestyleTracker(aParentRestyler.mRestyleTracker)
, mSelectorsForDescendants(aParentRestyler.mSelectorsForDescendants)
, mTreeMatchContext(aParentRestyler.mTreeMatchContext)
Expand Down Expand Up @@ -1197,8 +1188,6 @@ ElementRestyler::ElementRestyler(nsPresContext* aPresContext,
, mChangeList(aChangeList)
, mHintsHandledByAncestors(aHintsHandledByAncestors)
, mHintsHandledBySelf(nsChangeHint(0))
, mParentFrameHintsNotHandledForDescendants(nsChangeHint(0))
, mHintsNotHandledForDescendants(nsChangeHint(0))
, mRestyleTracker(aRestyleTracker)
, mSelectorsForDescendants(aSelectorsForDescendants)
, mTreeMatchContext(aTreeMatchContext)
Expand Down Expand Up @@ -1284,7 +1273,6 @@ ElementRestyler::CaptureChange(nsStyleContext* aOldContext,

nsChangeHint ourChange =
aOldContext->CalcStyleDifference(aNewContext,
mParentFrameHintsNotHandledForDescendants,
aEqualStructs,
aSamePointerStructs);
NS_ASSERTION(!(ourChange & nsChangeHint_AllReflowHints) ||
Expand Down Expand Up @@ -1332,10 +1320,6 @@ ElementRestyler::CaptureChange(nsStyleContext* aOldContext,
} else {
LOG_RESTYLE("change has already been handled");
}
mHintsNotHandledForDescendants |=
NS_HintsNotHandledForDescendantsIn(ourChange);
LOG_RESTYLE("mHintsNotHandledForDescendants = %s",
GeckoRestyleManager::ChangeHintToString(mHintsNotHandledForDescendants).get());
}

class MOZ_RAII AutoSelectorArrayTruncater final
Expand Down Expand Up @@ -2481,13 +2465,6 @@ ElementRestyler::RestyleSelf(nsIFrame* aSelf,
canStopWithStyleChange = false;
}

if (providerFrame != aSelf->GetParent()) {
// We don't actually know what the parent style context's
// non-inherited hints were, so assume the worst.
mParentFrameHintsNotHandledForDescendants =
nsChangeHint_Hints_NotHandledForDescendants;
}

LOG_RESTYLE("parentContext = %p", parentContext);

// do primary context
Expand Down Expand Up @@ -2680,7 +2657,7 @@ ElementRestyler::RestyleSelf(nsIFrame* aSelf,
// same-style continuations (bug 918064), we need to check again here to
// determine whether it is safe to stop restyling.
if (result == RestyleResult::eStop) {
oldContext->CalcStyleDifference(newContext, nsChangeHint(0),
oldContext->CalcStyleDifference(newContext,
&equalStructs,
&samePointerStructs);
if (equalStructs != NS_STYLE_INHERIT_MASK) {
Expand Down
3 changes: 0 additions & 3 deletions layout/base/GeckoRestyleManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,9 +725,6 @@ class ElementRestyler final
// initially zero, and accumulates hints for each same-style continuation
// and {ib} split sibling we restyle for the node.
nsChangeHint mHintsHandledBySelf;
// See nsStyleContext::CalcStyleDifference
nsChangeHint mParentFrameHintsNotHandledForDescendants;
nsChangeHint mHintsNotHandledForDescendants;
RestyleTracker& mRestyleTracker;
nsTArray<nsCSSSelector*>& mSelectorsForDescendants;
TreeMatchContext& mTreeMatchContext;
Expand Down
4 changes: 0 additions & 4 deletions layout/build/nsLayoutModule.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,10 +377,6 @@ Initialize()
return rv;
}

#ifdef DEBUG
nsStyleContext::AssertStyleStructMaxDifferenceValid();
#endif

return NS_OK;
}

Expand Down
1 change: 0 additions & 1 deletion layout/generic/nsFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10186,7 +10186,6 @@ nsFrame::UpdateStyleOfChildAnonBox(nsIFrame* aChildFrame,
uint32_t equalStructs, samePointerStructs; // Not used, actually.
nsChangeHint childHint = aChildFrame->StyleContext()->CalcStyleDifference(
newContext,
NS_HintsNotHandledForDescendantsIn(aHintForThisFrame),
&equalStructs,
&samePointerStructs);
if (childHint) {
Expand Down
5 changes: 0 additions & 5 deletions layout/style/ServoBindings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,17 +306,12 @@ Gecko_CalcStyleDifference(nsStyleContext* aOldStyleContext,
MOZ_ASSERT(aOldStyleContext);
MOZ_ASSERT(aComputedValues);

// Pass the safe thing, which causes us to miss a potential optimization. See
// bug 1289863.
nsChangeHint forDescendants = nsChangeHint_Hints_NotHandledForDescendants;

// Eventually, we should compute things out of these flags like
// ElementRestyler::RestyleSelf does and pass the result to the caller to
// potentially halt traversal. See bug 1289868.
uint32_t equalStructs, samePointerStructs;
nsChangeHint result =
aOldStyleContext->CalcStyleDifference(aComputedValues,
forDescendants,
&equalStructs,
&samePointerStructs);

Expand Down
70 changes: 7 additions & 63 deletions layout/style/nsStyleContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -987,15 +987,11 @@ nsStyleContext::ApplyStyleFixups(bool aSkipParentDisplayBasedStyleFixup)
template<class StyleContextLike>
nsChangeHint
nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
nsChangeHint aParentHintsNotHandledForDescendants,
uint32_t* aEqualStructs,
uint32_t* aSamePointerStructs)
{
PROFILER_LABEL("nsStyleContext", "CalcStyleDifference",
js::ProfileEntry::Category::CSS);
MOZ_ASSERT(NS_IsHintSubset(aParentHintsNotHandledForDescendants,
nsChangeHint_Hints_NotHandledForDescendants),
"caller is passing inherited hints, but shouldn't be");

static_assert(nsStyleStructID_Length <= 32,
"aEqualStructs is not big enough");
Expand All @@ -1013,34 +1009,17 @@ nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
// we could later get a small change in one of those structs that we
// don't want to miss.

// If our sources are the same, then any differences in style data
// are already accounted for by differences on ancestors. We know
// this because CalcStyleDifference is always called on two style
// contexts that point to the same element, so we know that our
// position in the style context tree is the same and our position in
// the rule node tree (if applicable) is also the same.
// However, if there were noninherited style change hints on the
// parent, we might produce these same noninherited hints on this
// style context's frame due to 'inherit' values, so we do need to
// compare.
// (Things like 'em' units are handled by the change hint produced
// by font-size changing, so we don't need to worry about them like
// 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
// all of the other style structs too, even if the new style context has
// the same source as the old one.
// FIXME(heycam): We should just do the comparison in
// nsStyleVariables::CalcDifference, returning NeutralChange if there are
// any Variables differences.
const nsStyleVariables* thisVariables = PeekStyleVariables();
if (thisVariables) {
structsFound |= NS_STYLE_INHERIT_BIT(Variables);
const nsStyleVariables* otherVariables = aNewContext->StyleVariables();
if (thisVariables->mVariables == otherVariables->mVariables) {
*aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
} else {
compare = true;
}
} else {
*aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
Expand All @@ -1054,33 +1033,14 @@ nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
if (this##struct_) { \
structsFound |= NS_STYLE_INHERIT_BIT(struct_); \
const nsStyle##struct_* other##struct_ = aNewContext->Style##struct_(); \
nsChangeHint maxDifference = nsStyle##struct_::MaxDifference(); \
nsChangeHint differenceAlwaysHandledForDescendants = \
nsStyle##struct_::DifferenceAlwaysHandledForDescendants(); \
if (this##struct_ == other##struct_) { \
/* The very same struct, so we know that there will be no */ \
/* differences. */ \
*aEqualStructs |= NS_STYLE_INHERIT_BIT(struct_); \
} else if (compare || \
((maxDifference & ~differenceAlwaysHandledForDescendants) & \
aParentHintsNotHandledForDescendants)) { \
nsChangeHint difference = \
this##struct_->CalcDifference(*other##struct_ EXTRA_DIFF_ARGS); \
NS_ASSERTION(NS_IsHintSubset(difference, maxDifference), \
"CalcDifference() returned bigger hint than " \
"MaxDifference()"); \
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_ EXTRA_DIFF_ARGS); \
NS_ASSERTION(NS_IsHintSubset(difference, maxDifference), \
"CalcDifference() returned bigger hint than " \
"MaxDifference()"); \
hint |= difference; \
if (!difference) { \
*aEqualStructs |= NS_STYLE_INHERIT_BIT(struct_); \
} \
Expand All @@ -1091,11 +1051,9 @@ nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
styleStructCount++; \
PR_END_MACRO

// 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
// that subsumes their MaxDifference. (As the hints get
// finer-grained, this optimization is becoming less useful, though.)
// FIXME: 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".
#define EXTRA_DIFF_ARGS /* nothing */
DO_STRUCT_DIFFERENCE(Display);
DO_STRUCT_DIFFERENCE(XUL);
Expand Down Expand Up @@ -1248,12 +1206,10 @@ nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,

nsChangeHint
nsStyleContext::CalcStyleDifference(nsStyleContext* aNewContext,
nsChangeHint aParentHintsNotHandledForDescendants,
uint32_t* aEqualStructs,
uint32_t* aSamePointerStructs)
{
return CalcStyleDifferenceInternal(aNewContext,
aParentHintsNotHandledForDescendants,
aEqualStructs,
aSamePointerStructs);
}
Expand Down Expand Up @@ -1290,13 +1246,11 @@ class MOZ_STACK_CLASS FakeStyleContext

nsChangeHint
nsStyleContext::CalcStyleDifference(const ServoComputedValues* aNewComputedValues,
nsChangeHint aParentHintsNotHandledForDescendants,
uint32_t* aEqualStructs,
uint32_t* aSamePointerStructs)
{
FakeStyleContext newContext(aNewComputedValues);
return CalcStyleDifferenceInternal(&newContext,
aParentHintsNotHandledForDescendants,
aEqualStructs,
aSamePointerStructs);
}
Expand Down Expand Up @@ -1531,16 +1485,6 @@ nsStyleContext::CombineVisitedColors(nscolor *aColors, bool aLinkIsVisited)
}

#ifdef DEBUG
/* static */ void
nsStyleContext::AssertStyleStructMaxDifferenceValid()
{
#define STYLE_STRUCT(name, checkdata_cb) \
MOZ_ASSERT(NS_IsHintSubset(nsStyle##name::DifferenceAlwaysHandledForDescendants(), \
nsStyle##name::MaxDifference()));
#include "nsStyleStructList.h"
#undef STYLE_STRUCT
}

/* static */ const char*
nsStyleContext::StructName(nsStyleStructID aSID)
{
Expand Down
11 changes: 1 addition & 10 deletions layout/style/nsStyleContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,18 +395,12 @@ class nsStyleContext final
* This method returns a change hint (see nsChangeHint.h). All change
* hints apply to the frame and its later continuations or ib-split
* siblings. Most (all of those except the "NotHandledForDescendants"
* hints) also apply to all descendants. The caller must pass in any
* non-inherited hints that resulted from the parent style context's
* style change. The caller *may* pass more hints than needed, but
* must not pass less than needed; therefore if the caller doesn't
* know, the caller should pass
* nsChangeHint_Hints_NotHandledForDescendants.
* hints) also apply to all descendants.
*
* aEqualStructs must not be null. Into it will be stored a bitfield
* representing which structs were compared to be non-equal.
*/
nsChangeHint CalcStyleDifference(nsStyleContext* aNewContext,
nsChangeHint aParentHintsNotHandledForDescendants,
uint32_t* aEqualStructs,
uint32_t* aSamePointerStructs);

Expand All @@ -415,14 +409,12 @@ class nsStyleContext final
* a full-fledged style context.
*/
nsChangeHint CalcStyleDifference(const ServoComputedValues* aNewComputedValues,
nsChangeHint aParentHintsNotHandledForDescendants,
uint32_t* aEqualStructs,
uint32_t* aSamePointerStructs);

private:
template<class StyleContextLike>
nsChangeHint CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
nsChangeHint aParentHintsNotHandledForDescendants,
uint32_t* aEqualStructs,
uint32_t* aSamePointerStructs);

Expand Down Expand Up @@ -506,7 +498,6 @@ class nsStyleContext final

#ifdef DEBUG
void List(FILE* out, int32_t aIndent, bool aListDescendants = true);
static void AssertStyleStructMaxDifferenceValid();
static const char* StructName(nsStyleStructID aSID);
static bool LookupStruct(const nsACString& aName, nsStyleStructID& aResult);
#endif
Expand Down
Loading

0 comments on commit cf56de7

Please sign in to comment.