Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes #15065

Manishearth · 2017-01-17T06:52:13Z

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

This change is

highfive · 2017-01-17T06:52:16Z

Heads up! This PR modifies the following files:

@bholley: components/style/servo/media_queries.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/longhand/effects.mako.rs, components/style/values/specified/image.rs, components/style/values/specified/basic_shape.rs, components/style/values/computed/length.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/properties/longhand/inherited_table.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/values/specified/length.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/values/specified/position.rs, components/style/values/computed/image.rs, components/style/viewport.rs
@KiChjang: components/script/dom/element.rs
@fitzgen: components/script/dom/element.rs
@emilio: components/style/servo/media_queries.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/longhand/effects.mako.rs, components/style/values/specified/image.rs, components/style/values/specified/basic_shape.rs, components/style/values/computed/length.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/properties/longhand/inherited_table.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/values/specified/length.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/values/specified/position.rs, components/style/values/computed/image.rs, components/style/viewport.rs

highfive · 2017-01-17T06:52:17Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

Manishearth · 2017-01-17T06:53:48Z

For reference this brings the numbers down to

120 / clip
88 / transform-origin
72 / text-overflow
64 / perspective-origin
64 / border-top-right-radius
64 / border-top-left-radius
64 / border-bottom-right-radius
64 / border-bottom-left-radius
60 / -servo-text-decorations-in-effect
48 / outline-color
48 / border-top-color
48 / border-spacing
48 / border-right-color
48 / border-left-color
48 / border-inline-start-color
48 / border-inline-end-color
48 / border-bottom-color
48 / border-block-start-color
48 / border-block-end-color
48 / background-color
40 / word-spacing
40 / vertical-align
40 / line-height
40 / color
32 / width
32 / top
32 / text-indent
32 / right
32 / perspective
32 / padding-top
32 / padding-right
32 / padding-left
32 / padding-inline-start
32 / padding-inline-end
32 / padding-bottom
32 / padding-block-start
32 / padding-block-end
32 / offset-inline-start
32 / offset-inline-end
32 / offset-block-start
32 / offset-block-end
32 / min-width
32 / min-inline-size
32 / min-height
32 / min-block-size
32 / max-width
32 / max-inline-size
32 / max-height
32 / max-block-size
32 / margin-top
32 / margin-right
32 / margin-left
32 / margin-inline-start
32 / margin-inline-end
32 / margin-bottom
32 / margin-block-start
32 / margin-block-end
32 / letter-spacing
32 / left
32 / inline-size
32 / height
32 / font-size
32 / flex-basis
32 / content
32 / column-width
32 / column-gap
32 / bottom
32 / border-top-width
32 / border-right-width
32 / border-left-width
32 / border-inline-start-width
32 / border-inline-end-width
32 / border-bottom-width
32 / border-block-start-width
32 / border-block-end-width
32 / block-size
24 / transition-timing-function
24 / transition-property
24 / transition-duration
24 / transition-delay
24 / transform
24 / text-shadow
24 / quotes
24 / outline-width
24 / outline-offset
24 / list-style-image
24 / font-family
24 / filter
24 / counter-reset
24 / counter-increment
24 / box-shadow
24 / background-size
24 / background-repeat
24 / background-position-y
24 / background-position-x
24 / background-origin
24 / background-image
24 / background-clip
24 / background-attachment
24 / animation-timing-function
24 / animation-play-state
24 / animation-name
24 / animation-iteration-count
24 / animation-fill-mode
24 / animation-duration
24 / animation-direction
24 / animation-delay

Manishearth · 2017-01-17T07:13:45Z

For Gecko, we still have a 416-byte clip-path and 232 byte border-image-source. Might be worth boxing these (and some of the other top properties). Also should investigate the sizes of properties in vector longhands; don't want too much memory to be used in case of capacity preallocation.

416 / clip-path
232 / border-image-source
88 / transform-origin
72 / text-overflow
64 / perspective-origin
64 / border-top-right-radius
64 / border-top-left-radius
64 / border-bottom-right-radius
64 / border-bottom-left-radius
64 / -moz-outline-radius-topright
64 / -moz-outline-radius-topleft
64 / -moz-outline-radius-bottomright
64 / -moz-outline-radius-bottomleft
48 / text-emphasis-color
48 / text-decoration-color
48 / stop-color
48 / outline-color
48 / list-style-image
48 / lighting-color
48 / flood-color
48 / column-rule-color
48 / border-top-color
48 / border-spacing
48 / border-right-color
48 / border-left-color
48 / border-inline-start-color
48 / border-inline-end-color
48 / border-bottom-color
48 / border-block-start-color
48 / border-block-end-color
48 / background-color
48 / -webkit-text-stroke-color
48 / -webkit-text-fill-color
48 / -moz-binding
40 / word-spacing
40 / vertical-align
40 / scroll-snap-points-y
40 / scroll-snap-points-x
40 / line-height
40 / grid-row-start
40 / grid-row-end
40 / grid-column-start
40 / grid-column-end
40 / color
32 / width
32 / top
32 / text-indent
32 / text-emphasis-style
32 / right
32 / perspective
32 / padding-top
32 / padding-right
32 / padding-left
32 / padding-inline-start
32 / padding-inline-end
32 / padding-bottom
32 / padding-block-start
32 / padding-block-end
32 / offset-inline-start
32 / offset-inline-end
32 / offset-block-start
32 / offset-block-end
32 / min-width
32 / min-inline-size
32 / min-height
32 / min-block-size
32 / max-width
32 / max-inline-size
32 / max-height
32 / max-block-size
32 / margin-top
32 / margin-right
32 / margin-left
32 / margin-inline-start
32 / margin-inline-end
32 / margin-bottom
32 / margin-block-start
32 / margin-block-end
32 / letter-spacing
32 / left
32 / inline-size
32 / height
32 / font-size
32 / flex-basis
32 / cursor
32 / content
32 / column-width
32 / column-rule-width
32 / column-gap
32 / bottom
32 / border-top-width
32 / border-right-width
32 / border-left-width
32 / border-inline-start-width
32 / border-inline-end-width
32 / border-image-slice
32 / border-bottom-width
32 / border-block-start-width
32 / border-block-end-width
32 / block-size
32 / -webkit-text-stroke-width
24 / transition-timing-function
24 / transition-property
24 / transition-duration
24 / transition-delay
24 / transform
24 / text-shadow
24 / quotes
24 / outline-width
24 / mask-size
24 / mask-repeat
24 / mask-position
24 / mask-origin
24 / mask-mode
24 / mask-image
24 / mask-composite
24 / mask-clip
24 / font-family
24 / filter
24 / box-shadow
24 / border-image-width
24 / border-image-outset
24 / background-size
24 / background-repeat
24 / background-position-y
24 / background-position-x
24 / background-origin
24 / background-image
24 / background-clip
24 / background-blend-mode
24 / background-attachment
24 / animation-timing-function
24 / animation-play-state
24 / animation-name
24 / animation-iteration-count
24 / animation-fill-mode
24 / animation-duration
24 / animation-direction
24 / animation-delay

emilio · 2017-01-17T07:23:59Z

Do you hace any runtime numbers for this? Length is a commonly used type on the stack too, and these are a lot of new allocations.

Manishearth · 2017-01-17T07:25:38Z

Length only allocates if there's a calc in there. Calcs are pretty rare, and worth allocating for.

heycam · 2017-01-17T07:28:16Z

In general, boxing CalcLoP sounds fine to me.

FWIW, I don't see any value in having CalcLoP store each of its possible components as a separate field, rather than a list of some sort, when it will be very rare to have anything other than a single length unit plus a percentage, and since we don't need random access to the fields anyway (we just take them one by one for serialization, and converting to computed values, AFAICT).

heycam · 2017-01-17T07:55:09Z

@bors-servo r+

bors-servo · 2017-01-17T07:55:10Z

📌 Commit cff661c has been approved by heycam

bors-servo · 2017-01-17T07:56:18Z

⌛ Testing commit cff661c with merge 8824dcc...

emilio · 2017-01-17T08:10:14Z

@Manishearth could you squash the commits if you have the time or this doesn't merge on first try? I don't love "Fix tidy" style commits.

emilio · 2017-01-17T08:10:47Z

(Thought that's also debatable, I guess?) Anyway :)

bors-servo · 2017-01-17T08:13:32Z

💔 Test failed - mac-dev-unit

Manishearth · 2017-01-17T08:31:22Z

@bors-servo r=heycam

bors-servo · 2017-01-17T08:31:22Z

📌 Commit 79d85e5 has been approved by heycam

bors-servo · 2017-01-17T09:01:03Z

⌛ Testing commit 79d85e5 with merge 808f3d4...

@heycam

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15065)

Ms2ger · 2017-01-17T09:10:26Z

@bors-servo r-

Please do squash.

wafflespeanut · 2017-01-17T09:15:43Z

FWIW, I don't see any value in having CalcLoP store each of its possible components as a separate field, rather than a list of some sort, when it will be very rare to have anything other than a single length unit plus a percentage, and since we don't need random access to the fields anyway (we just take them one by one for serialization, and converting to computed values, AFAICT).

@heycam You mean something like this?

bors-servo · 2017-01-17T10:18:13Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

…on(s)

Manishearth · 2017-01-17T16:10:20Z

@bors-servo r=heycam

bors-servo · 2017-01-17T16:10:21Z

📌 Commit 79d85e5 has been approved by heycam

KiChjang · 2017-01-17T19:09:49Z

@bors-servo r-
Not squashed yet as requested by @Ms2ger.

Manishearth · 2017-01-17T19:13:18Z

@bors-servo r=heycam

git push -f

bors-servo · 2017-01-17T19:13:19Z

📌 Commit f59557d has been approved by heycam

bors-servo · 2017-01-17T19:49:53Z

⌛ Testing commit f59557d with merge f010fb5...

@heycam

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15065)

bors-servo · 2017-01-17T20:44:50Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned heycam Jan 17, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 17, 2017

Manishearth mentioned this pull request Jan 17, 2017

Make PropertyDeclaration tinier #15061

Closed

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 17, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 17, 2017

Manishearth force-pushed the box-calclop branch from cff661c to 79d85e5 Compare January 17, 2017 08:31

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 17, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 17, 2017

Box CalcLengthOrPercentage to reduce inline size of PropertyDeclarati…

f59557d

…on(s)

Manishearth force-pushed the box-calclop branch from 79d85e5 to f59557d Compare January 17, 2017 19:12

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 17, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 17, 2017

bors-servo merged commit f59557d into servo:master Jan 17, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 17, 2017

Manishearth deleted the box-calclop branch January 18, 2017 03:17

Manishearth mentioned this pull request Feb 1, 2017

Box larger specified values to avoid memmove impact #15322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes #15065

Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes #15065

Manishearth commented Jan 17, 2017 •

edited by larsbergstrom

highfive commented Jan 17, 2017

highfive commented Jan 17, 2017

Manishearth commented Jan 17, 2017 •

edited

Manishearth commented Jan 17, 2017 •

edited

emilio commented Jan 17, 2017

Manishearth commented Jan 17, 2017

heycam commented Jan 17, 2017

heycam commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

emilio commented Jan 17, 2017

emilio commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Manishearth commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Ms2ger commented Jan 17, 2017

wafflespeanut commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Manishearth commented Jan 17, 2017

bors-servo commented Jan 17, 2017

KiChjang commented Jan 17, 2017

Manishearth commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes #15065

Use Box<CalcLengthOrPercentage> in specified values to avoid bloating inline sizes #15065

Conversation

Manishearth commented Jan 17, 2017 • edited by larsbergstrom

highfive commented Jan 17, 2017

highfive commented Jan 17, 2017

Manishearth commented Jan 17, 2017 • edited

Manishearth commented Jan 17, 2017 • edited

emilio commented Jan 17, 2017

Manishearth commented Jan 17, 2017

heycam commented Jan 17, 2017

heycam commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

emilio commented Jan 17, 2017

emilio commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Manishearth commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Ms2ger commented Jan 17, 2017

wafflespeanut commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Manishearth commented Jan 17, 2017

bors-servo commented Jan 17, 2017

KiChjang commented Jan 17, 2017

Manishearth commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

bors-servo commented Jan 17, 2017

Manishearth commented Jan 17, 2017 •

edited by larsbergstrom

Manishearth commented Jan 17, 2017 •

edited

Manishearth commented Jan 17, 2017 •

edited