Reduce amount of Mako-generated code in style #17703

SimonSapin · 2017-07-12T23:12:06Z

In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as LonghandId.

Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1377262

Machine code size is measured with bloaty, looking at the total "VM size" in order to exclude debug info (which is stripped in binaries shipped to users).

mach build-geckolib --release
bloaty target/geckolib/release/libgeckoservo.a

Initial size was 9.56Mi. Successive commits in this PR bring it to 9.55Mi, 9.51Mi, 9.44Mi, 9.31Mi, 8.89Mi, 8.89Mi, and 7.54Mi. Total savings: 2.02 MiB.

In an optimized Firefox build with mach build && mach package, the size of obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so goes from 109.11 MiB to 107.49 MiB, saving 1.62 MiB. I don’t really know how to explain the difference. Does libgeckoservo.a contain code that ends up not being used in libxul.so?

This change is

SimonSapin · 2017-07-12T23:15:00Z

@bors-servo try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6adcbd0a5dbfdc242f647921e7c13816f96506

Another factor that could explain some of the difference: mach build-geckolib uses Rust 1.17.0 (which it downloads itself), whereas Firefox uses the system’s Rust which on my build machine is 1.18.0. The latter adds field re-ordering. If that or other new optimizations reduce the size of all Rust code, the saving would be roughly proportionally reduced.

bors-servo · 2017-07-12T23:15:05Z

⌛ Trying commit 1d2f11b with merge 322c3a4...

Reduce amount of Mako-generated code in style In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as `LonghandId`. Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1377262 Machine code size is measured with [bloaty](https://github.com/google/bloaty), looking at the total "VM size" in order to exclude debug info (which is stripped in binaries shipped to users). ``` mach build-geckolib --release bloaty target/geckolib/release/libgeckoservo.a ``` Initial size was 9.56Mi. Successive commits in this PR bring it to 9.55Mi, 9.51Mi, 9.44Mi, 9.31Mi, 8.89Mi, 8.89Mi, and 7.54Mi. Total savings: 2.02 MiB. In an optimized Firefox build with `mach build && mach package`, the size of `obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so` goes from 109.11 MiB to 107.49 MiB, saving 1.62 MiB. I don’t really know how to explain the difference. Does `libgeckoservo.a` contain code that ends up not being used in `libxul.so`?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17703)

SimonSapin · 2017-07-12T23:19:22Z

In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as LonghandId.

There may be more of this still doable. Look for Mako % for loops that generate code for each CSS property in *.mako.rs files.

bors-servo · 2017-07-13T01:20:04Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

SimonSapin · 2017-07-13T07:37:58Z

@bors-servo try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b85a225800237623f182143ec99ac2413dd27988

bors-servo · 2017-07-13T07:38:02Z

⌛ Trying commit a234f53 with merge 395b5ba...

Reduce amount of Mako-generated code in style In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as `LonghandId`. Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1377262 Machine code size is measured with [bloaty](https://github.com/google/bloaty), looking at the total "VM size" in order to exclude debug info (which is stripped in binaries shipped to users). ``` mach build-geckolib --release bloaty target/geckolib/release/libgeckoservo.a ``` Initial size was 9.56Mi. Successive commits in this PR bring it to 9.55Mi, 9.51Mi, 9.44Mi, 9.31Mi, 8.89Mi, 8.89Mi, and 7.54Mi. Total savings: 2.02 MiB. In an optimized Firefox build with `mach build && mach package`, the size of `obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so` goes from 109.11 MiB to 107.49 MiB, saving 1.62 MiB. I don’t really know how to explain the difference. Does `libgeckoservo.a` contain code that ends up not being used in `libxul.so`?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17703)

bors-servo · 2017-07-13T08:32:24Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

SimonSapin · 2017-07-13T11:37:56Z

@bors-servo try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf7c837af04ba713034f5ea3bb42f06cb7475c2

bors-servo · 2017-07-13T11:38:00Z

⌛ Trying commit 5922bc0 with merge 9b14022...

Reduce amount of Mako-generated code in style In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as `LonghandId`. Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1377262 Machine code size is measured with [bloaty](https://github.com/google/bloaty), looking at the total "VM size" in order to exclude debug info (which is stripped in binaries shipped to users). ``` mach build-geckolib --release bloaty target/geckolib/release/libgeckoservo.a ``` Initial size was 9.56Mi. Successive commits in this PR bring it to 9.55Mi, 9.51Mi, 9.44Mi, 9.31Mi, 8.89Mi, 8.89Mi, and 7.54Mi. Total savings: 2.02 MiB. In an optimized Firefox build with `mach build && mach package`, the size of `obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so` goes from 109.11 MiB to 107.49 MiB, saving 1.62 MiB. I don’t really know how to explain the difference. Does `libgeckoservo.a` contain code that ends up not being used in `libxul.so`?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17703)

SimonSapin · 2017-07-13T12:53:58Z

Looks like buildbot just didn’t schedule some of the jobs.

@bors-servo try

bors-servo · 2017-07-13T12:54:03Z

⌛ Trying commit db6cdf4 with merge 778582c...

Reduce amount of Mako-generated code in style In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as `LonghandId`. Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1377262 Machine code size is measured with [bloaty](https://github.com/google/bloaty), looking at the total "VM size" in order to exclude debug info (which is stripped in binaries shipped to users). ``` mach build-geckolib --release bloaty target/geckolib/release/libgeckoservo.a ``` Initial size was 9.56Mi. Successive commits in this PR bring it to 9.55Mi, 9.51Mi, 9.44Mi, 9.31Mi, 8.89Mi, 8.89Mi, and 7.54Mi. Total savings: 2.02 MiB. In an optimized Firefox build with `mach build && mach package`, the size of `obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so` goes from 109.11 MiB to 107.49 MiB, saving 1.62 MiB. I don’t really know how to explain the difference. Does `libgeckoservo.a` contain code that ends up not being used in `libxul.so`?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17703)

bors-servo · 2017-07-13T14:19:19Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

… rather than generating similar code for each shorthand.

… rather than generating similar code for every longhand property.

… rather than generating similar code for each shorthand.

… rather than generating similar code for every longhand property

…methods.

…s Err

SimonSapin · 2017-07-13T14:24:14Z

Tests are green, fixup commits are squashed, this is good to review and land.

emilio

nit: Typo in the first commit message (s/genrated/generated)

emilio · 2017-07-13T14:26:10Z

components/style/properties/helpers.mako.rs

+                    maybe_wm = ", wm" if property.logical else ""
+                    maybe_cacheable = ", cacheable" if property.has_uncacheable_values == "True" else ""
+                    props_need_device = "content list_style_type".split() if product == "gecko" else []
+                    maybe_device = ", context.device" if property.ident in props_need_device else ""


These hacks should go away eventually... sigh.

I recommend adding ?w=1 to github URLs to make diff not show as modified lines where only indentation changed.

Yeah, I know this is just moving code around... It's just that seeing this makes me sad.

emilio

Mostly minor comments. r=me

emilio · 2017-07-13T14:32:59Z

components/style/properties/properties.mako.rs

                                shorthands::${shorthand.ident}::parse_into(declarations, context, input)
-                                    .map_err(|_| PropertyDeclarationParseError::InvalidValue("${shorthand.ident}".into()))
+                                    .map_err(|_| PropertyDeclarationParseError::InvalidValue(
+                                        "${shorthand.ident}".into()


nit: I suspect this really wants do do shorthand.name... But this is preexisting anyway.

emilio · 2017-07-13T14:34:42Z

components/style/properties/helpers.mako.rs

                }
+                specified.map(|s| PropertyDeclaration::${property.camel_case}(s))


nit: I think you can just remove the closure (specified.map(PropertyDeclaration::${property.camel_case}))

Done so in a following commit.

emilio · 2017-07-13T14:36:36Z

components/style/properties/helpers.mako.rs

+                if specified.is_err() {
+                    while let Ok(_) = input.next() {}  // Look for var() after the error.
+                }
+                let var = input.seen_var_functions();


can't this just be:

if specified.is_err() { while let Ok(_) = input.next() {} // Look for var() after the error. if input.seen_var_functions() { // ... } }

Seems easier to understand off-hand, wdyt?

This code is rewritten in a following commit.

emilio · 2017-07-13T14:38:43Z

components/style/properties/properties.mako.rs

+                        }
+                    })
+                }).map(|declaration| {
+                    declarations.push(declaration)


nit: I don't love to use map with side effects, but I guess it's not a big deal.

emilio · 2017-07-13T14:42:58Z

components/style/properties/helpers.mako.rs

+                    maybe_wm = ", wm" if property.logical else ""
+                    maybe_cacheable = ", cacheable" if property.has_uncacheable_values == "True" else ""
+                    props_need_device = "content list_style_type".split() if product == "gecko" else []
+                    maybe_device = ", context.device" if property.ident in props_need_device else ""


Yeah, I know this is just moving code around... It's just that seeing this makes me sad.

emilio · 2017-07-13T14:45:45Z

components/style/properties/properties.mako.rs

@@ -552,6 +462,11 @@ impl LonghandId {
        }
    }

+    fn inherited(&self) -> bool {


I think we could try to use this more in a few places, I think it could reduce a bit of code size here and there too.

emilio · 2017-07-13T14:47:50Z

components/style/properties/properties.mako.rs

@@ -2838,16 +2821,18 @@ pub fn apply_declarations<'a, F, I>(device: &Device,
                         CascadeLevel::UserNormal |
                         CascadeLevel::UserImportant |
                         CascadeLevel::UAImportant) {
+                let non_transparent_background;


nit: Use match here?

let non_transparent_background = match *declaration { PropertyDeclaration::BackgroundColor(ref color) => color.is_non_transparent(), _ => continue, };

SimonSapin · 2017-07-13T14:59:15Z

@bors-servo r=emilio

bors-servo · 2017-07-13T14:59:16Z

📌 Commit 09d6c83 has been approved by emilio

Reduce amount of Mako-generated code in style In multiple places, instead of generated similar code for each CSS property, have a single code path that uses an enum such as `LonghandId`. Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1377262 Machine code size is measured with [bloaty](https://github.com/google/bloaty), looking at the total "VM size" in order to exclude debug info (which is stripped in binaries shipped to users). ``` mach build-geckolib --release bloaty target/geckolib/release/libgeckoservo.a ``` Initial size was 9.56Mi. Successive commits in this PR bring it to 9.55Mi, 9.51Mi, 9.44Mi, 9.31Mi, 8.89Mi, 8.89Mi, and 7.54Mi. Total savings: 2.02 MiB. In an optimized Firefox build with `mach build && mach package`, the size of `obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so` goes from 109.11 MiB to 107.49 MiB, saving 1.62 MiB. I don’t really know how to explain the difference. Does `libgeckoservo.a` contain code that ends up not being used in `libxul.so`?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17703)

bors-servo · 2017-07-13T15:51:58Z

⌛ Testing commit 09d6c83 with merge 68ae7ce...

bors-servo · 2017-07-13T16:53:36Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 68ae7ce to master...

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

SimonSapin force-pushed the code-size branch from 5922bc0 to db6cdf4 Compare July 13, 2017 12:53

SimonSapin added 9 commits July 13, 2017 16:23

Property validity checks: generated tables rather than genrated code

3075746

CSS-wide keywords parsing in shorthand: use a single code path with IDs

a5b80e2

… rather than generating similar code for each shorthand.

CSS-wide keywords parsing in longhands: use a single code path with IDs

8a8614e

… rather than generating similar code for every longhand property.

var() functions in shorthands: use a single code path with IDs

252e52e

… rather than generating similar code for each shorthand.

var() functions in longhands: use a single code path with IDs

54f1325

… rather than generating similar code for every longhand property

Move property value parsing from longhand/shorthand ID into separate …

42813bc

…methods.

Fix handling of all shorthand with var() functions

66c357f

Fix shorthand parsing pushing declarations when parse_entirely return…

3d3c196

…s Err

Merge per-property substitute_variables* functions into one.

34c5a21

SimonSapin force-pushed the code-size branch from db6cdf4 to 34c5a21 Compare July 13, 2017 14:23

emilio reviewed Jul 13, 2017

View reviewed changes

emilio approved these changes Jul 13, 2017

View reviewed changes

Use match instead of if let of one-line branches.

09d6c83

highfive assigned emilio Jul 13, 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 Jul 13, 2017

bors-servo merged commit 09d6c83 into master Jul 13, 2017

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

SimonSapin deleted the code-size branch July 13, 2017 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce amount of Mako-generated code in style #17703

Reduce amount of Mako-generated code in style #17703

SimonSapin commented Jul 12, 2017 •

edited by larsbergstrom

Loading

SimonSapin commented Jul 12, 2017

bors-servo commented Jul 12, 2017

SimonSapin commented Jul 12, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

emilio left a comment

emilio Jul 13, 2017

SimonSapin Jul 13, 2017

emilio Jul 13, 2017

emilio left a comment

emilio Jul 13, 2017

emilio Jul 13, 2017

SimonSapin Jul 13, 2017

emilio Jul 13, 2017

SimonSapin Jul 13, 2017

emilio Jul 13, 2017

emilio Jul 13, 2017

emilio Jul 13, 2017

emilio Jul 13, 2017

SimonSapin Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

		}
		specified.map(\|s\| PropertyDeclaration::${property.camel_case}(s))

Reduce amount of Mako-generated code in style #17703

Reduce amount of Mako-generated code in style #17703

Conversation

SimonSapin commented Jul 12, 2017 • edited by larsbergstrom Loading

SimonSapin commented Jul 12, 2017

bors-servo commented Jul 12, 2017

SimonSapin commented Jul 12, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 13, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSapin commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 13, 2017

SimonSapin commented Jul 12, 2017 •

edited by larsbergstrom

Loading