1855887 - Consider improving lazy clone of inherited.inherited in CustomPropertiesBuilder

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Description

[Frédéric Wang Nélar]

Currently, MutableCustomProperties starts with custom_properties.inherited starts as None and performing lazy clone of inherited.inherited.

More specifically, we are doing a deep clone in cascade if a property may affect the style:

https://searchfox.org/mozilla-central/rev/b0d28aecd58cbd2db00974db2ef8456856169fb4/servo/components/style/custom_properties.rs#802

and a shallow clone in the build function if the map is still None at the end:

https://searchfox.org/mozilla-central/rev/b0d28aecd58cbd2db00974db2ef8456856169fb4/servo/components/style/custom_properties.rs#925

With the work done in bug 1840478, this is going to change:

1. custom_properties.inherited can have initial values on the root (we assert inherited.inherited is None, so here we don't need to worry about that map later). Otherwise custom_properties.inherited remains None and we perform lazy clone.
2. cascade may executed for a non-inherited property. In that case the deep clone mentioned above is not necessary and done too early.
3. In build, custom_properties.inherited may still be None but we can't just do a shallow clone anymore. Indeed, a deep clone is still necessary to make the substitute_all code properly work.

However, it could be possible to remove the deep clones mentioned above. Instead make sure CustomPropertiesBuilder always use inherited.inherited as a fallback for the values of custom_properties.inherited (*). If in the build function we decide at the end not to do a lazy clone then inherited.inherited will have to be merged into the custom_properties.inherited before returning.

The disadvantage is that we have to potentially query too maps (custom_properties.inherited and inherited.inherited) to read a value. Also I'm not sure what's more costly between (A) doing a deep clone of inherited.inherited and adding values afterwards or (B) adding values first and then merging inherited.inherited at the end.

(*) for the shared substitution code, if ReadOnlyCustomProperties is MutableCustomProperties we would need to somewhat pass the inherited.inherited map to be able to perform the fallback ; if it is a ComputedCustomProperties that's not necessary.

Comment 1

[Frédéric Wang Nélar]

Attachment: Bug 1855887 - Improve handling of keywords in substitute_references_in_value_and_apply. r=emilio

In bug 1840478, two follow-up TODO questions were left in
substitute_references_in_value_and_apply regarding handling of CSS
keywords such as 'unset'. For non-inherited custom properties, we don't
need to put any value in the map of computed properties when that value
is the initial one, so just remove it. For inherited custom properties,
we could probably do the same when that value is the inherited one and
postpone actually deep cloning the inherited map at the end of the
build method. Comment is updated to refer to bug 1855887 instead.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f55472aa743
Improve handling of keywords in substitute_references_in_value_and_apply. r=emilio

Comment 3

[Frédéric Wang Nélar]

Attachment: WIP: Bug 1855887 - postpone clone of inherited.inherited in CustomPropertiesBuilder::build(). r=emilio

Comment 4

[Frédéric Wang Nélar]

@Emilio: I attached a patch to try and get rid of the early deep clones of the inherited map. However, I noticed using absent values to temporarily represent inherited values of inherited properties is problematic because they are also used for guaranteed-invalid values...

Actually, I also noticed that similarly using absent values to represent initial values of non-inherited properties is problematic for the same reason, see my attempt on bug 1855946.

So I'm not sure really sure which representation would minimize memory use / deep clones here. And as a minor issue, we shouldn't forget to choose a representation that would help on bug 1855629...

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1855887 - Simplify custom property representation and handling. r=fredw

We can use Arc::make_mut for writes so that the inherited properties are
lazily cloned when needed, and simplify a bit the code while at it.

If make_mut is somehow too expensive we can optimize it further.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Ok, I think I have a plan that both simplifies the setup and should achieve this while at it. Let me know what you think.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f186d047c1ca
Simplify custom property representation and handling. r=fredw

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2f55472aa743

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f186d047c1ca