Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes #15953

bholley · 2017-03-15T06:01:23Z

Right now they don't, which means that we have four properties making PropertyDeclaration 16 bytes bigger than it should be.

I'm not sure if there's a better way to get these tests to run against stylo than to hoist them into the properties file, but I couldn't figure it out. This seems good enough.

This change is

MozReview-Commit-ID: GMd1Huv8N5s

highfive · 2017-03-15T06:01:26Z

Heads up! This PR modifies the following files:

@emilio: components/style/properties/longhand/list.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/inherited_svg.mako.rs

bholley · 2017-03-15T06:01:53Z

r? @Manishearth

bholley · 2017-03-15T06:01:59Z

@bors-servo try

bors-servo · 2017-03-15T06:02:04Z

⌛ Trying commit 502e393 with merge 537cd0e...

Make size_of tests measure stylo Right now they don't, which means that we have four properties making PropertyDeclaration 16 bytes bigger than it should be. I'm not sure if there's a better way to get these tests to run against stylo than to hoist them into the properties file, but I couldn't figure it out. This seems good enough.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15953)

bholley · 2017-03-15T06:03:13Z

I guess the other question is whether non-stylo test runs actually run tests from components/style or whether they're strictly limited to tests/unit/style. We certainly shouldn't disable these tests for normal servo runs.

CC @wafflespeanut

bholley · 2017-03-15T06:03:40Z

Also, the reason those properties are so large is that we have a bunch of extra fields on URL specified values for stylo.

jdm · 2017-03-15T06:03:43Z

components/style/properties/properties.mako.rs

+    let new = size_of::<PropertyDeclaration>();
+    if new < old {
+        panic!("Your changes have decreased the stack size of PropertyDeclaration enum from {} to {}. \
+                Good work! Please update the size in tests/unit/style/size_of.rs.",


This message references the old file.

jdm · 2017-03-15T06:03:52Z

components/style/properties/properties.mako.rs

+                These enum is present in large quantities in the style, and increasing the size \
+                may negatively affect style system performance. Please consider using `boxed=\"True\"` in \
+                the longhand If you feel that the increase is necessary, update to the new size in \
+                tests/unit/style/size_of.rs.",


This message references the old file.

Manishearth · 2017-03-15T06:18:12Z

I guess the other question is whether non-stylo test runs actually run tests from components/style or whether they're strictly limited to tests/unit/style

It doesn't. There aren't supposed to be tests in components/, the only reason style is an exception is that bindgen generates layout tests inline and we have to run those for stylo. So you probably want to export a function that can be called from tests/unit and components/style

bors-servo · 2017-03-15T06:41:57Z

☀️ 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
State: approved= try=True

wafflespeanut · 2017-03-15T07:18:42Z

We should consider having another size_of module in stylo_tests and enable both testing and gecko features for the style crate. Then, we can test this using ./mach test-stylo

SimonSapin · 2017-03-15T08:39:01Z

@bholley This PR adds a #[test] function in the style crate, but I think that test will never be run in our current CI. To run it you’d need to change something in python/servo so that either ./mach test-unit or ./mach test-stylo also runs cargo test -p style.

However that involves recompiling the entire style crate (with --test passed to rustc) which take some time. To avoid that the test can be in a separate crate, either similar to tests/unit/style (as a Cargo "package" with its own Cargo.toml file) or what Cargo calls an integration test (which is a separate crate from the point of view of rustc, but not a separate package as far as Cargo is concerned).

wafflespeanut · 2017-03-15T08:42:00Z

... or, simply make a copy of size_of (or move it) to stylo tests, so that it can be tested there.

MozReview-Commit-ID: KnXLfHfITyG

MozReview-Commit-ID: KapDMqX6OjH

bholley · 2017-03-15T18:20:49Z

Fixed up. r? @Manishearth

Manishearth · 2017-03-15T18:23:21Z

@bors-servo r+

bors-servo · 2017-03-15T18:23:22Z

📌 Commit 6744ed1 has been approved by Manishearth

bors-servo · 2017-03-15T18:23:26Z

⌛ Testing commit 6744ed1 with merge f5c67fd...

Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes Right now they don't, which means that we have four properties making PropertyDeclaration 16 bytes bigger than it should be. I'm not sure if there's a better way to get these tests to run against stylo than to hoist them into the properties file, but I couldn't figure it out. This seems good enough.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15953)

bors-servo · 2017-03-15T19:00:18Z

☀️ 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
Approved by: Manishearth
Pushing f5c67fd to master...

Tweak thresholds.

418f829

MozReview-Commit-ID: GMd1Huv8N5s

highfive assigned mbrubeck Mar 15, 2017

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

highfive assigned Manishearth and unassigned mbrubeck Mar 15, 2017

jdm requested changes Mar 15, 2017

View reviewed changes

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 15, 2017

bholley added 2 commits March 15, 2017 10:47

Box marker properties and list-style-image on stylo.

9f44fd2

MozReview-Commit-ID: KnXLfHfITyG

Run size_of tests with test-stylo.

6744ed1

MozReview-Commit-ID: KapDMqX6OjH

bholley force-pushed the size_of_stylo branch from 502e393 to 6744ed1 Compare March 15, 2017 18:19

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 15, 2017

bholley changed the title ~~Make size_of tests measure stylo~~ Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes Mar 15, 2017

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 15, 2017

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 15, 2017

bors-servo merged commit 6744ed1 into servo:master Mar 15, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes #15953

Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes #15953

bholley commented Mar 15, 2017 •

edited by larsbergstrom

highfive commented Mar 15, 2017

bholley commented Mar 15, 2017

bholley commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bholley commented Mar 15, 2017

bholley commented Mar 15, 2017

jdm Mar 15, 2017

jdm Mar 15, 2017

Manishearth commented Mar 15, 2017

bors-servo commented Mar 15, 2017

wafflespeanut commented Mar 15, 2017 •

edited

SimonSapin commented Mar 15, 2017

wafflespeanut commented Mar 15, 2017

bholley commented Mar 15, 2017

Manishearth commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes #15953

Make size_of tests measure stylo and reduce the size of stylo PropertyDeclarations by 16 bytes #15953

Conversation

bholley commented Mar 15, 2017 • edited by larsbergstrom

highfive commented Mar 15, 2017

bholley commented Mar 15, 2017

bholley commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bholley commented Mar 15, 2017

bholley commented Mar 15, 2017

jdm Mar 15, 2017

Choose a reason for hiding this comment

jdm Mar 15, 2017

Choose a reason for hiding this comment

Manishearth commented Mar 15, 2017

bors-servo commented Mar 15, 2017

wafflespeanut commented Mar 15, 2017 • edited

SimonSapin commented Mar 15, 2017

wafflespeanut commented Mar 15, 2017

bholley commented Mar 15, 2017

Manishearth commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bholley commented Mar 15, 2017 •

edited by larsbergstrom

wafflespeanut commented Mar 15, 2017 •

edited