Closed Bug 1289622 Opened 4 years ago Closed 4 years ago

Add machinery to invoke CalcStyleDifference directly on a ServoComputedValues and stash the result on the old nsStyleContext


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: bholley, Assigned: bholley)




(4 files)

Can't hook this up to anything until Emilio's traversal code lands, but might as well get it in the tree.
Note: On part 3, I decided to store the change hint on the old style context instead of on nsINode, since we that restricts the memory footprint closer to the cases where we actually need it.
Comment on attachment 8774939 [details] [diff] [review]
Part 3 - Add the ability to store change hints on the style context. v1

Review of attachment 8774939 [details] [diff] [review]:

This won't work if we ever start sharing nsStyleContexts.  I'm not sure if we will, but just in case, can you please assert in nsStyleContext::StoreChangeHint that !IsShared() and in nsStyleContext::FindChildWithRules (where we set NS_STYLE_IS_SHARED) that !mHasStoredChangeHint.

r=me knowing that we'll likely want to avoid storing the change hint here in the long term (hopefully there's a better way to get the change hint to the change hint processing phase without needing to have storage that persists beyond the restyling process).

::: layout/style/nsStyleContext.h
@@ +526,5 @@
>    }
>    mozilla::NonOwningStyleContextSource StyleSource() const { return mSource.AsRaw(); }
> +  void StoreChangeHint(nsChangeHint aHint)

This hunk needs to be in #ifdef MOZ_STYLO?
Attachment #8774939 - Flags: review?(cam) → review+
Attachment #8774937 - Flags: review?(cam) → review+
Comment on attachment 8774938 [details] [diff] [review]
Part 2 - Add a ServoComputedValues* overload for CalcStyleDifference. v1

Review of attachment 8774938 [details] [diff] [review]:

::: layout/style/nsStyleContext.cpp
@@ +1225,5 @@
>  }
> +#ifdef MOZ_STYLO
> +
> +class MOZ_STACK_CLASS FakeStyleContext {

Nit: "{" on new line.

@@ +1240,5 @@
> +    return nullptr;
> +  }
> +
> +  #define STYLE_STRUCT(name_, checkdata_cb_)              \
> +  const nsStyle##name_ * Style##name_() {               \

Nit: keep these backslashes aligned.
Attachment #8774938 - Flags: review?(cam) → review+
Comment on attachment 8774940 [details] [diff] [review]
Part 4 - Add an API for Servo traversal to call CalcStyleDifference. v1

Review of attachment 8774940 [details] [diff] [review]:

::: layout/style/ServoBindings.cpp
@@ +190,5 @@
> +{
> +  nsStyleContext* oldContext = aElement->GetPrimaryFrame()->StyleContext();
> +
> +  // XXXbholley: This is probably wrong. What's the right thing to pass here?
> +  nsChangeHint forDescendants = nsChangeHint_Hints_NotHandledForDescendants;

This argument to CalcStyleDifference is used as an optimization to know whether we can skip calling a given struct's CalcDifference method.  If we had a change on an ancestor that resulted in a change hint that was handled for descendants, then if the only change hint we might generate for the current node is that same hint, then we don't need to call CalcDifference.

For example, if the parent change generated nsChangeHint_RepaintFrame from one of its structs, and the only possible hint that could be generated from a given struct on this node is nsChangeHint_RepaintFrame, then we can skip calling CalcDifference for that struct.  This is because repainting the parent will handle repainting any descendants.

So if we don't track and accumulate which hints we have recorded for ancestors, the safe thing to do is use nsChangeHint_Hints_NotHandledForDescendants, which means "an ancestor hasn't generated any of these hints that could allow us to opt in to this optimization".  But let's file a bug to do this so we don't forget to do it at some point.
Attachment #8774940 - Flags: review?(cam) → review+
Filed bug 1289860 for the nsChangeHint storage and bug 1289863 for the CalcStyleDifference optimization.
Pushed by
Delegate CalcStyleDifference to a templated helper. r=heycam
Add a ServoComputedValues* overload for CalcStyleDifference. r=heycam
Add the ability to store change hints on the style context. r=heycam
Add an API for Servo traversal to call CalcStyleDifference. r=heycam
Blocks: stylo-incremental
No longer blocks: stylo
See Also: → 1380133
You need to log in before you can comment on or make changes to this bug.