Open Bug 1394297 Opened 7 years ago Updated 2 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix

People

(Reporter: heycam, Unassigned)

References

Details

Attachments

(2 files)

Bug 1394297 - style: Don't mutate style structs when attempting to store the same value.
59 bytes, text/x-review-board-request
emilio
: review+
Details
test
277 bytes, text/html
Details 
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 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 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 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+
Going to ask for re-review due to the layout/style/ changes.
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!
Hmm, this one is still very orange too. :-(
Attached file 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-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: affected → wontfix
Assignee: cam → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: