1399011 - stylo: Swap RestyleFlags for ElementDataFlags and eliminate RestyleData

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Bobby Holley (:bholley)]

Right now the only flags in ElementData live in RestyleData. This means we need to abuse abstractions a bit in order to have non-restyle-related flags, because adding another byte anywhere else within ElementData will cause it to grow.

Unfortunately, just moving the flags outside of RestyleData still causes ElementData to grow, presumably because the size of RestyleData gets rounded up. Given that the structure is just conceptual at this point and doesn't have any special heap-allocation semantics, it's easiest to just get rid of it.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist flags out of RestyleData. v1

MozReview-Commit-ID: 8emE83lykh3

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Eliminate RestyleData entirely. v1

Without this change, the previous commit increases the size of ElementData.

MozReview-Commit-ID: 87BZuXINiT9

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8906911 [details] [diff] [review]
Part 1 - Hoist flags out of RestyleData. v1

Review of attachment 8906911 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with that bit addressed.

::: servo/components/style/matching.rs
@@ +605,5 @@
>          cascade_requirement = cmp::max(
>              cascade_requirement,
>              self.accumulate_damage_for(
>                  context.shared,
> +                data.skip_applying_damage(),

Why not just passing `data` down? I'd actually prefer to keep the check as it was, since it seemed more obvious it was a Gecko-only optimization.

@@ +706,5 @@
>                           shared_context: &SharedStyleContext,
>                           restyle: &mut RestyleData,
>                           old_values: Option<&ComputedValues>,
>                           new_values: &ComputedValues,
> +                         pseudo: Option<&PseudoElement>,

I think this is dead code?

I'll remove it in https://github.com/servo/servo/pull/18456, that way you can just pass data down.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8906912 [details] [diff] [review]
Part 2 - Eliminate RestyleData entirely. v1

Review of attachment 8906912 [details] [diff] [review]:
-----------------------------------------------------------------

Not super-happy, but shrug.

::: servo/components/style/matching.rs
@@ +628,5 @@
>                  (&Some(ref old), &Some(ref new)) => {
>                      self.accumulate_damage_for(
>                          context.shared,
>                          data.skip_applying_damage(),
> +                        &mut data.damage,

ditto, you can just pass `data` here I think.

Well, maybe not since it may be borrowed up... That kinda sucks, but ok, I guess it's fine to do this skip_applying_damage() method.

Comment 6

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/18465

Comment 7

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/c8cb0cd2d1e5