Make PropertyDeclaration tinier #15061
Labels
A-content/css
Interacting with CSS from web content (parsing, serializing, introspection)
C-is-this-done
It is unclear if the issue has been resolved
I-perf-bloat
Unnecessary memory usage.
Comments
Manishearth
added
A-content/css
|
These are the sizes for the computed values Not as bad, but the computed values add up, unlike the specified values, where only the max is used. However, we never store these computed values in Stylo except for animation. Still, we should fix some of these for Servo memory usage. |
3 tasks
bors-servo
pushed a commit
that referenced
this issue
Jan 17, 2017
Cleaning up CalcLengthOrPercentage <!-- Please describe your changes on the following line: --> We don't really need enum variants in `CalcLengthOrPercentage`. Given that we already have the information (whether it's `vw`, `ch`, etc.) in the struct fields, we could just store `Option<CSSFloat>` there, and modify our `ToCss` implementation a bit. cc #15061 r? @Manishearth or anyone interested --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Either: --> - [x] These changes do not require tests because it's a refactor <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15063) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Jan 17, 2017
Cleaning up CalcLengthOrPercentage <!-- Please describe your changes on the following line: --> We don't really need enum variants in `CalcLengthOrPercentage`. Given that we already have the information (whether it's `vw`, `ch`, etc.) in the struct fields, we could just store `Option<CSSFloat>` there, and modify our `ToCss` implementation a bit. cc #15061 r? @Manishearth or anyone interested --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Either: --> - [x] These changes do not require tests because it's a refactor <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15063) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Jan 17, 2017
Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes For #15061 CalcLOP is a large struct, and gets used quite often. While #15063 reduces its size a bit, it will still be much larger than any of the other variants in the `specified::Length*` types, so it will still bloat sizes, especially for specified values that contain many lengths. This change boxes it in the length types, so that it just takes one word. r? @heycam <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15065) <!-- Reviewable:end -->
|
Thank you for doing this analysis. Boxing data in large enum variants to make the enum smaller sounds good. As you said though, computed values are not typically used in enums so boxing them wouldn’t help much with total memory usage, I think. |
bors-servo
pushed a commit
that referenced
this issue
Jan 17, 2017
Cleaning up CalcLengthOrPercentage <!-- Please describe your changes on the following line: --> We don't really need enum variants in `CalcLengthOrPercentage`. Given that we already have the information (whether it's `vw`, `ch`, etc.) in the struct fields, we could just store `Option<CSSFloat>` there, and modify our `ToCss` implementation a bit. cc #15061 r? @Manishearth or anyone interested --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Either: --> - [x] These changes do not require tests because it's a refactor <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15063) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Jan 17, 2017
Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes For #15061 CalcLOP is a large struct, and gets used quite often. While #15063 reduces its size a bit, it will still be much larger than any of the other variants in the `specified::Length*` types, so it will still bloat sizes, especially for specified values that contain many lengths. This change boxes it in the length types, so that it just takes one word. r? @heycam <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15065) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Jan 28, 2017
Introduce the `NoCalcLength` <!-- Please describe your changes on the following line: --> I began this for making the `CalcLengthOrPercentage` represent `LengthOrPercentage` (instead of the enum we already have), but only later did I realize that it will make `LengthOrPercentageOrFoo` types fatty (which is the problem we're trying to avoid - #15061) and so, I dropped that attempt. Along the way, I introduced an internal type for `Length`, for representing all its non-calc variants (which are `Copy`). We could still have this type for the `LengthOrPercentageOrFoo` types which don't really need `Length` since they already have their own variants for calc. r? @Manishearth @emilio @SimonSapin or anyone interested --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Either: --> - [x] These changes do not require tests because it's a refactor <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15115) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Jan 28, 2017
Introduce the `NoCalcLength` <!-- Please describe your changes on the following line: --> I began this for making the `CalcLengthOrPercentage` represent `LengthOrPercentage` (instead of the enum we already have), but only later did I realize that it will make `LengthOrPercentageOrFoo` types fatty (which is the problem we're trying to avoid - #15061) and so, I dropped that attempt. Along the way, I introduced an internal type for `Length`, for representing all its non-calc variants (which are `Copy`). We could still have this type for the `LengthOrPercentageOrFoo` types which don't really need `Length` since they already have their own variants for calc. r? @Manishearth @emilio @SimonSapin or anyone interested --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Either: --> - [x] These changes do not require tests because it's a refactor <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15115) <!-- Reviewable:end -->
|
@SimonSapin This can be considered done, right? |
|
Yes. We do box some variants of this enum, and have a test to ensure we box exactly those above a certain (configurable) size. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We should be doing more boxing here. It's more than a thousand bytes of inline space (
mem::size_of) right now.Properties larger than 10 bytes in Servo:
Properties larger than 10 bytes in Stylo:
In particular,
clipandclip-pathare huge. We should determine a cutoff and start boxing there. Possibly do the same for the computed values. And add an autogenerated test that asserts that thesize_ofsizes are below this number.Many of these large numbers are due to
Lengthbeing large (clipis just 4 lengths and it is 492 bytes large). Even just boxingCalcLengthOrPercentagewould reduce numbers significantly here. @wafflespeanut is already refactoring some of this.cc @bholley @SimonSapin @heycam
h/t @nnethercote for finding this
The text was updated successfully, but these errors were encountered: