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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Can't hook this up to anything until Emilio's traversal code lands, but might as well get it in the tree.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Delegate CalcStyleDifference to a templated helper. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a ServoComputedValues* overload for CalcStyleDifference. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add the ability to store change hints on the style context. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add an API for Servo traversal to call CalcStyleDifference. v1

Comment 5

[Bobby Holley (:bholley)]

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 6

[Cameron McCormack (:heycam)]

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?

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[Bobby Holley (:bholley)]

Filed bug 1289860 for the nsChangeHint storage and bug 1289863 for the CalcStyleDifference optimization.

Comment 10

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c1f468ca6fd

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/176fb76f315f
Delegate CalcStyleDifference to a templated helper. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5e026f11aa9
Add a ServoComputedValues* overload for CalcStyleDifference. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/8801607503fe
Add the ability to store change hints on the style context. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/196ddcbf5225
Add an API for Servo traversal to call CalcStyleDifference. r=heycam

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/176fb76f315f
https://hg.mozilla.org/mozilla-central/rev/d5e026f11aa9
https://hg.mozilla.org/mozilla-central/rev/8801607503fe
https://hg.mozilla.org/mozilla-central/rev/196ddcbf5225