stylo: don't mutate style structs when attempting to store the same value

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P4
normal
ASSIGNED
9 months ago
7 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
Per bug 1367854 comment 24, we can check whether the value we're about to store in a style struct is the same as the existing value, and if we haven't already mutated the struct, we can just skip it.

For the HTML spec with STYLO_THREADS=1, this results in a 3.33 MB saving in style struct memory usage.
Comment hidden (mozreview-request)

Comment 3

9 months ago
mozreview-review
Comment on attachment 8901657 [details]
Bug 1394297 - style: Don't mutate style structs when attempting to store the same value.

https://reviewboard.mozilla.org/r/173086/#review178430

Looks good, mod comments. I'm curious to know the answer to the first question before r+'ing though. We can do something about the `transition-` / `animation-` functions, though it'd be somewhat hacky.

::: servo/components/style/properties/properties.mako.rs:2688
(Diff revision 1)
>          }
>      }
>  
> +    <%
> +        uncloneable_properties = """
> +animation-delay

Is there a reason we need another list instead of just using `property.need_clone`?

Also, I suspect this may have a negative perf impact, but maybe it's not too much and that's ok.

::: servo/components/style/properties/properties.mako.rs:2717
(Diff revision 1)
> +    %>
> +
>      % for property in data.longhands:
>      % if property.ident != "font_size":
>      /// Inherit `${property.ident}` from our parent style.
> -    #[allow(non_snake_case)]
> +    #[allow(non_snake_case, unused_variables)]

I don't see any unused variables, neither here or below, could we revert this and the same change the other functions?

::: servo/components/style/properties/properties.mako.rs:3514
(Diff revision 1)
>          }
>      % endif
>  
> +    % for style_struct in data.active_style_structs():
> +    if builder.get_${style_struct.name_lower}_if_mutated().is_some() {
> +        println!("computed unique ${style_struct.name}");

remove.
Attachment #8901657 - Flags: review?(emilio)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8901657 [details]
Bug 1394297 - style: Don't mutate style structs when attempting to store the same value.

https://reviewboard.mozilla.org/r/173086/#review178432

Actually, r=me with the comments addressed, if we can just use `need_clone`.

Comment 5

9 months ago
mozreview-review
Comment on attachment 8901657 [details]
Bug 1394297 - style: Don't mutate style structs when attempting to store the same value.

https://reviewboard.mozilla.org/r/173086/#review178434
Attachment #8901657 - Flags: review+
(Assignee)

Comment 6

9 months ago
Going to ask for re-review due to the layout/style/ changes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Just for reference:

11:09 <emilio> heycam: looks fine, though it seems quite wasteful to get the atom as a string only to reatomize it a few lines later
11:10 <heycam> emilio: hmm
11:10 <emilio> heycam: seems like it wouldn't be hard to fix
11:10 <heycam> emilio: with a separate FFI function
11:10 <emilio> heycam: yes
11:10 <heycam> ok :-)
11:11 <emilio> heycam: ok, r=me with that bit fixed, thanks!
(Assignee)

Comment 11

9 months ago
Hmm, this one is still very orange too. :-(
Comment hidden (mozreview-request)
(Assignee)

Comment 13

9 months ago
Created attachment 8902100 [details]
test

Here's a small test that demonstrates at least one failure mode of the patch.  Both boxes should show no border.  Notice that the order of the border-top-width and border-top-style declarations matters.  I think this is because if we cascade border-top-width first, it looks like we don't need to apply the declaration, because the existing reset struct border-top-width value is 0.  But that is because of the border-width fixups we do when border-style is none.

We could make sure that we apply border-style first, by sorting the declarations passed in to apply_declarations.  But I'm not sure what the best way to encode those dependencies in the code would be, such that we wouldn't get it wrong.
Other way to fix this is including both the specified and computed border in the computed value of the property, like we do
for align-items.
Priority: P1 → P4
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.