Closed Bug 1377594 Opened 8 years ago Closed 8 years ago

stylo: 374k in animated_properties functions

Categories

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

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

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: away, Unassigned)

References

(Blocks 1 open bug)

Details

84330 1 style::properties::animated_properties::AnimationValue::from_declaration 43903 10 style::properties::animated_properties::{{impl}}::add_weighted 32878 1 style::properties::animated_properties::AnimatedProperty::from_animatable_longhand 28675 2 style::properties::animated_properties::{{impl}}::fmt 26516 1 style::properties::animated_properties::{{impl}}::accumulate 23763 2 style::properties::animated_properties::{{impl}}::add 22291 1 style::properties::animated_properties::AnimationValue::uncompute 19170 1 style::properties::animated_properties::AnimationValue::from_computed_values 13954 2 style::properties::animated_properties::{{impl}}::eq 12443 1 style::properties::animated_properties::{{impl}}::ne 12172 1 style::properties::animated_properties::AnimatedProperty::does_animate 11500 5 style::properties::animated_properties::{{impl}}::compute_distance 9723 2 style::properties::animated_properties::{{impl}}::clone 8395 6 style::properties::animated_properties::{{impl}}::from 7375 1 style::properties::animated_properties::TransitionProperty::parse 4614 1 style::properties::animated_properties::add_weighted_transform_lists 4061 1 style::properties::animated_properties::decompose_3d_matrix 2347 1 style::properties::animated_properties::{{impl}}::to_css<collections::string::String> First column is total size, second is count -- so for example there are ten functions called "add_weighted" with a combined size of 43k. Am I correct to guess that animation is relatively rare, so we could potentially trade a little speed for space here?
Wow, I knew this setup was bad, but I didn't realize it was this bad. In particular, nearly all properties have a default implementation of 'accumulate' and 'add' (that re-uses add_weighted) but the two combined take up more space than add_weighted alone. I think Nox has been looking into how to clean this up so he might have some ideas.
(In reply to David Major [:dmajor] from comment #0) 84330 1 style::properties::animated_properties::AnimationValue::from_declaration If we have substitute_variables_slow(LonghandId&, ...) function, we can reduce the function size 1/3? 32878 1 style::properties::animated_properties::AnimatedProperty::from_animatable_longhand 12172 1 style::properties::animated_properties::AnimatedProperty::does_animate For these AnimatedProperty fuctions, we can eliminate them completely and drop AnimatedProperty altogether if we do some tweaks in needs_transitions_update_per_property.
The AnimationValue could be generated in something a bit smaller, for example instead of: BorderTopLeftRadius(longhands::border_top_left_radius::computed_value::T), /// border-top-right-radius BorderTopRightRadius(longhands::border_top_right_radius::computed_value::T), /// border-bottom-right-radius BorderBottomRightRadius(longhands::border_bottom_right_radius::computed_value::T), /// border-bottom-left-radius BorderBottomLeftRadius(longhands::border_bottom_left_radius::computed_value::T), We could have: BorderCornerRadius(Corner, CornerRadius), Basically, reuse the same variant for properties that have the same type of values and add an inner C-like enum discriminant.
Depends on: 1377680
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > 32878 1 > style::properties::animated_properties::AnimatedProperty:: > from_animatable_longhand > 12172 1 > style::properties::animated_properties::AnimatedProperty::does_animate > > For these AnimatedProperty fuctions, we can eliminate them completely and > drop AnimatedProperty altogether if we do some tweaks in > needs_transitions_update_per_property. Filed for this part. bug 1377680.
Priority: -- → P3
Summary: 374k in animated_properties functions in stylo → stylo: 374k in animated_properties functions
(In reply to David Major [:dmajor] from comment #0) > 84330 1 style::properties::animated_properties::AnimationValue::from_declaration I think this part has been reduced since Simon's effort in https://github.com/servo/servo/pull/17703 .
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > (In reply to David Major [:dmajor] from comment #0) > > 84330 1 style::properties::animated_properties::AnimationValue::from_declaration > > I think this part has been reduced since Simon's effort in > https://github.com/servo/servo/pull/17703 . That function is still quite large, and animated_properties's total contribution is still pretty high. From today's m-c: 74104 style::properties::animated_properties::AnimationValue::from_declaration 42303 style::properties::animated_properties::AnimationValue::uncompute 37432 style::properties::animated_properties::{{impl}}::fmt 36520 style::properties::animated_properties::{{impl}}::add_weighted 25930 style::properties::animated_properties::AnimationValue::from_computed_values 24775 style::properties::animated_properties::{{impl}}::accumulate 23971 style::properties::animated_properties::{{impl}}::add 23417 style::properties::animated_properties::{{impl}}::eq 22445 style::properties::animated_properties::{{impl}}::ne 13649 style::properties::animated_properties::{{impl}}::clone 13145 style::properties::animated_properties::{{impl}}::compute_distance 8678 style::properties::animated_properties::TransitionProperty::parse 6186 style::properties::animated_properties::{{impl}}::add_weighted 5605 style::properties::animated_properties::{{impl}}::from
Priority: P3 → --
Priority: -- → P3
A low-hanging fruit to shave off 22K would be to implement `PartialEq` ourselves to make `ne` return the negation of `eq`.
(In reply to Anthony Ramine [:nox] from comment #7) > A low-hanging fruit to shave off 22K would be to implement `PartialEq` > ourselves to make `ne` return the negation of `eq`. Sounds worth doing!
https://github.com/servo/servo/pull/18134 landed, which introduce a single `animate` method instead of the three very repetitive methods `add_weighted`, `accumulate` and `add`. I have no idea how the listing with the code sizes was generated, so I'll defer to someone else to see what changed.
(In reply to Anthony Ramine [:nox] from comment #9) > https://github.com/servo/servo/pull/18134 landed, which introduce a single > `animate` method instead of the three very repetitive methods > `add_weighted`, `accumulate` and `add`. I have no idea how the listing with > the code sizes was generated, so I'll defer to someone else to see what > changed. From today's Nightly: 83003 style::properties::animated_properties::AnimationValue::from_declaration 65960 style::properties::animated_properties::{{impl}}::animate 51787 style::properties::animated_properties::AnimationValue::uncompute 38252 style::properties::animated_properties::{{impl}}::fmt 28172 style::properties::animated_properties::AnimationValue::from_computed_values 24226 style::properties::animated_properties::{{impl}}::ne 23405 style::properties::animated_properties::{{impl}}::eq 15930 style::properties::animated_properties::{{impl}}::clone 13163 style::properties::animated_properties::{{impl}}::compute_squared_distance 8765 style::properties::animated_properties::TransitionProperty::parse 6794 style::properties::animated_properties::{{impl}}::animate 5627 style::properties::animated_properties::{{impl}}::from It looks like all functions have grown a bit in the last month (or maybe this is because I'm looking at an official nightly versus previously I was building my own m-c? in any case it doesn't seem large enough to worry about) but at least `animate` is smaller than the sum of `add_weighted` + `accumulate` + `add` were before.
Just realised that given I derived a lot of these impls, and that given the types on which I derived them aren't defined in animated_properties, this blurs the picture given I moved some of the code out of the module without killing it.
https://github.com/servo/servo/pull/18396 gives a nice win for ServoComputedData::to_declaration_block.
(In reply to Josh Matthews [:jdm] from comment #12) > https://github.com/servo/servo/pull/18396 gives a nice win for > ServoComputedData::to_declaration_block. Nice! Do you think there are more potential wins tracked in this bug, or shall we close it?
Flags: needinfo?(josh)
Yes, I have more pull requests incoming.
Flags: needinfo?(josh)
https://github.com/servo/servo/pull/18414 feels like it's the end of the low-hanging fruit on the animated_properties functions that are >5kb according bloaty. I tried a bunch of other experiments around extracting common code from match arms, or grouping match arms with identical types together, but they did not yield any change in code size.
(In reply to Josh Matthews [:jdm] from comment #15) > https://github.com/servo/servo/pull/18414 feels like it's the end of the > low-hanging fruit on the animated_properties functions that are >5kb > according bloaty. I tried a bunch of other experiments around extracting > common code from match arms, or grouping match arms with identical types > together, but they did not yield any change in code size. Ok - thanks for doing all that!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.