1448559 - Cleanup CalcStyleDifference.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

The aSamePointerStructs argument is unused now. Also, aIgnoreVariables can be true everywhere now, since variable changes can't generate change hints, and anonymous boxes and such don't care about whether they really changed or not.

We had this optimization inconsistently in some cases but not others.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1448559: Cleanup CalcStyleDifference.

The aSamePointerStructs argument is unused now.

Also, aIgnoreVariables can be true everywhere now, since variable changes can't
generate change hints, and anonymous boxes and such don't care about whether
they really changed or not.

Only one caller cares about struct equality, and that already compares variables
manually as an optimization on the rust side.

We had this optimization inconsistently in some cases but not others.

Review commit: https://reviewboard.mozilla.org/r/230870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230870/

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 8962059 [details]
Bug 1448559: Cleanup CalcStyleDifference.

https://reviewboard.mozilla.org/r/230870/#review236434

::: commit-message-f2dd1:10
(Diff revision 1)
> +Also, aIgnoreVariables can be true everywhere now, since variable changes can't
> +generate change hints, and anonymous boxes and such don't care about whether
> +they really changed or not.
> +
> +Only one caller cares about struct equality, and that already compares variables
> +manually as an optimization on the rust side.

Comparing variables manually sounds like the exact opposite of an optimization...

::: layout/style/ComputedStyle.h:305
(Diff revision 1)
>     * aEqualStructs must not be null.  Into it will be stored a bitfield
>     * representing which structs were compared to be non-equal.
>     *
> -   * aIgnoreVariables indicates whether to skip comparing the Variables
> -   * struct.  This must only be true for Servo style contexts.  When
> -   * true, the Variables bit in aEqualStructs will be set.
> +   * CSS Variables are not compared here. Instead, the caller is responsible for
> +   * that when needed (basically only for elements). The Variables bit in
> +   * aEqualStructs is always set.

I don't understand the "basically only for elements" comment here. Maybe expand on that for ignorant people like me? :)

I think it would be good to add some of the rational from the commit message here to explain why they are not compared.

Also "The Variables bit in aEqualStructs is always set" sounds like "all callers add this", which is not what you mean. Maybe change that to "The Variables bit is added to aEqualStructs regardless of what callers pass".

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962059 [details]
Bug 1448559: Cleanup CalcStyleDifference.

https://reviewboard.mozilla.org/r/230870/#review236434

> Comparing variables manually sounds like the exact opposite of an optimization...

Well, the point is not having to compare it everywhere else where it's not needed, plus doing a FFI call, etc, and also do it only when only reset properties have changed, because if other inherited properties have changed we don't need that.

This is:

  https://searchfox.org/mozilla-central/rev/8220783953b0311e1d64c2366f732a159f05ed7e/servo/components/style/gecko/restyle_damage.rs#61

> I don't understand the "basically only for elements" comment here. Maybe expand on that for ignorant people like me? :)
> 
> I think it would be good to add some of the rational from the commit message here to explain why they are not compared.
> 
> Also "The Variables bit in aEqualStructs is always set" sounds like "all callers add this", which is not what you mean. Maybe change that to "The Variables bit is added to aEqualStructs regardless of what callers pass".

Anonymous boxes and such don't match rules, or their rules are under our control and shouldn't use custom properties. We don't inherit from pseudo-elements, so we don't need to know whether variables changed.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8962059 [details]
Bug 1448559: Cleanup CalcStyleDifference.

https://reviewboard.mozilla.org/r/230870/#review236464

r=me with the comments addressed.

Feel free to open new bugs or add new commits in this bug for addressing the last two comments.

::: layout/generic/nsFrame.cpp:10767
(Diff revision 1)
>    nsChangeHint childHint = aChildFrame->Style()->CalcStyleDifference(
>      aNewComputedStyle,
> -    &equalStructs,
> +    &equalStructs);

As far as you are here, probably
```c++
nsChangeHint childHint = aChildFrame->Style()->
  CalcStyleDifference(aNewComputedStyle, &equalStructs);
```
looks better?

::: layout/style/ComputedStyle.cpp
(Diff revision 1)
> -      Servo_ComputedValues_EqualCustomProperties(
> -        ComputedData(),
> -        aNewContext->ComputedData())) {

You are removing the only usage of this binding function. Can we remove it now?

::: layout/style/ComputedStyle.cpp:133
(Diff revision 1)
>  
> -  if (aIgnoreVariables ||
> -      Servo_ComputedValues_EqualCustomProperties(
> -        ComputedData(),
> -        aNewContext->ComputedData())) {
> -    *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
> +  *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);

This could be confusing. I'd prefer we don't set this bit at all, and instead handle it in where it actually cares.

(Actually, I guess we can make `aEqualStructs` optional given that majority of callsites don't really care about its value at all.)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Comment on attachment 8962059 [details]
> Bug 1448559: Cleanup CalcStyleDifference.
> 
> https://reviewboard.mozilla.org/r/230870/#review236464
> 
> r=me with the comments addressed.
> 
> Feel free to open new bugs or add new commits in this bug for addressing the
> last two comments.
> 
> ::: layout/generic/nsFrame.cpp:10767
> (Diff revision 1)
> >    nsChangeHint childHint = aChildFrame->Style()->CalcStyleDifference(
> >      aNewComputedStyle,
> > -    &equalStructs,
> > +    &equalStructs);
> 
> As far as you are here, probably
> ```c++
> nsChangeHint childHint = aChildFrame->Style()->
>   CalcStyleDifference(aNewComputedStyle, &equalStructs);
> ```
> looks better?

Sure, done.
 
> ::: layout/style/ComputedStyle.cpp
> (Diff revision 1)
> > -      Servo_ComputedValues_EqualCustomProperties(
> > -        ComputedData(),
> > -        aNewContext->ComputedData())) {
> 
> You are removing the only usage of this binding function. Can we remove it
> now?

Yes, will do as a followup.

> ::: layout/style/ComputedStyle.cpp:133
> (Diff revision 1)
> >  
> > -  if (aIgnoreVariables ||
> > -      Servo_ComputedValues_EqualCustomProperties(
> > -        ComputedData(),
> > -        aNewContext->ComputedData())) {
> > -    *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
> > +  *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
> 
> This could be confusing. I'd prefer we don't set this bit at all, and
> instead handle it in where it actually cares.
> 
> (Actually, I guess we can make `aEqualStructs` optional given that majority
> of callsites don't really care about its value at all.)

That requires a bit more thought because it may break the "reset only properties changed" check. I'll do that as a followup.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44a2d42ae375
Cleanup CalcStyleDifference. r=xidorn

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/44a2d42ae375