Make ToCss' SequenceWriter not monomorphise like a maniac anymore #19838

nox · 2018-01-22T19:24:07Z

This change is

highfive · 2018-01-22T19:24:11Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/computed/position.rs, ports/geckolib/glue.rs, components/style/stylesheets/style_rule.rs, components/style/properties/shorthand/background.mako.rs, components/style/gecko/url.rs and 83 more
@canaltinova: components/style/values/computed/position.rs, components/style/stylesheets/style_rule.rs, components/style/properties/shorthand/background.mako.rs, components/style/gecko/url.rs, components/style/values/computed/basic_shape.rs and 82 more
@emilio: components/style/values/computed/position.rs, ports/geckolib/glue.rs, components/style/stylesheets/style_rule.rs, components/style/properties/shorthand/background.mako.rs, components/style/gecko/url.rs and 83 more
@fitzgen: components/script/dom/cssstyledeclaration.rs, components/script/dom/mediaquerylist.rs, components/script/dom/medialist.rs
@KiChjang: components/script/dom/cssstyledeclaration.rs, components/script/dom/mediaquerylist.rs, components/script/dom/medialist.rs
@asajeffrey: components/script/dom/cssstyledeclaration.rs, components/script/dom/mediaquerylist.rs, components/script/dom/medialist.rs

highfive · 2018-01-22T19:24:15Z

Warning

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

nox · 2018-01-22T19:24:27Z

Cc @emilio @SimonSapin @bholley

nox · 2018-01-22T19:24:32Z

@bors-servo try

bors-servo · 2018-01-22T19:24:36Z

⌛ Trying commit a0cf705 with merge 2e32dbf...

Make ToCss' SequenceWriter not monomorphise like a maniac anymore

bors-servo · 2018-01-22T19:29:48Z

💔 Test failed - linux-rel-wpt

nox · 2018-01-22T19:35:39Z

@bors-servo try

bors-servo · 2018-01-22T19:35:43Z

⌛ Trying commit 4743dc3 with merge 3ff184c...

Make ToCss' SequenceWriter not monomorphise like a maniac anymore  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19838)

emilio

/me likes :)

Not sure if you want to wait for @SimonSapin's / @bholley's review or not, if you don't just feel free to land with r=me.

emilio · 2018-01-22T19:29:13Z

ports/geckolib/glue.rs

-                             None, None /* No extra custom properties */);
+        .single_value_to_css(
+            &get_property_id_from_nscsspropertyid!(property, ()),
+            &mut CssWriter::new(&mut string),


We should make this pass buffer directly. Mind filing an issue? Though I don't recall if @bholley's patch fixed this.

emilio · 2018-01-22T19:33:47Z

components/style_traits/values.rs

-            has_written: false,
+        let has_prefix = self.inner.prefix.is_some();
+        if !has_prefix {
+            self.inner.prefix = Some(self.separator);


oh, smart :)

emilio · 2018-01-22T19:35:17Z

components/style_traits/values.rs

+where
+    W: Write,
+{
+    /// Creates a new `CssWriter`.


nit: Maybe #[inline]

emilio · 2018-01-22T19:39:46Z

components/style_traits/values.rs

+/// A writer tailored for serialising CSS.
+///
+/// Coupled with SequenceWriter, this allows callers to transparently handle
+/// things like comma-separated values etc.


Maybe put an example of how this is used? I had to read it a bit carefully.

In particular, what state does prefix represent? If I'm not wrong, when writing a sequence:

We start with prefix = Some("").

On the first item, assuming it writes something, it takes the prefix.

On the second one, since there's no prefix, we set prefix: Some(sequence_separator), and reset it after the to_css call begins.

...

emilio · 2018-01-22T19:42:52Z

components/style_traits/values.rs

-            self.has_written = true;
+        item.to_css(&mut self.inner)?;
+        if !has_prefix {
+            self.inner.prefix = None;


I think it'd be nice, more as a self-documenting assert than as a sanity-check, that in this case self.inner.prefix is either None or self.separator before the assignment (since even though it can change down the line, it should always be mutated by another SequenceWriter, and thus resetted too.

bors-servo · 2018-01-22T19:55:58Z

💔 Test failed - linux-dev

nox · 2018-01-22T20:58:51Z

@bors-servo try

bors-servo · 2018-01-22T20:58:55Z

⌛ Trying commit 0bf70dc with merge f93f875...

Make ToCss' SequenceWriter not monomorphise like a maniac anymore  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19838)

bors-servo · 2018-01-22T21:54:56Z

☀️ 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

bors-servo · 2018-01-22T23:54:30Z

☔ The latest upstream changes (presumably #19841) made this pull request unmergeable. Please resolve the merge conflicts.

This more concrete wrapper type can write a prefix the very first time something is written to it. This allows removing plenty of useless monomorphisations caused by the former W/SequenceWriter<W> pair of types.

nox · 2018-01-23T09:50:35Z

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

bors-servo · 2018-01-23T10:01:18Z

💔 Test failed - mac-dev-unit

Now that SequenceWriter<W> does not monomorphise excessively, we can actually type check a derived ToCss without too much type recursion.

nox · 2018-01-23T10:09:23Z

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

@bors-servo try

bors-servo · 2018-01-23T10:09:28Z

⌛ Trying commit 42c8dc9 with merge e9b8fea...

Make ToCss' SequenceWriter not monomorphise like a maniac anymore  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19838)

bors-servo · 2018-01-23T10:36:31Z

💔 Test failed - arm32

emilio · 2018-01-23T12:34:20Z

@bors-servo r+

bors-servo · 2018-01-23T12:34:20Z

📌 Commit 42c8dc9 has been approved by emilio

nox · 2018-01-23T16:49:31Z

@bors-servo p=7

bors-servo · 2018-01-23T18:03:43Z

⌛ Testing commit 42c8dc9 with merge 6b2e528...

Make ToCss' SequenceWriter not monomorphise like a maniac anymore  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19838)

bors-servo · 2018-01-23T18:56:10Z

☀️ 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 6b2e528 to master...

ionutgoldan · 2018-01-25T10:06:19Z

Good news! This noticeably reduced build times on Windows.

== Change summary for alert #11256 (as of Tue, 23 Jan 2018 16:57:02 GMT) ==

Improvements:

9% build times windows2012-64 opt rusttests taskcluster-c4.4xlarge 2,321.94 -> 2,103.34
9% build times windows2012-32 opt rusttests taskcluster-c4.4xlarge 2,302.99 -> 2,090.30
9% build times windows2012-32 opt taskcluster-c4.4xlarge 2,580.50 -> 2,348.30
9% build times windows2012-64 opt taskcluster-c4.4xlarge 2,664.19 -> 2,437.35
8% build times windows2012-64 opt static-analysis taskcluster-c4.4xlarge2,580.59 -> 2,369.24
8% build times windows2012-32 opt static-analysis taskcluster-c4.4xlarge2,255.28 -> 2,079.92
4% build times windows2012-32 pgo taskcluster-c4.4xlarge 4,346.07 -> 4,185.03

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11256

highfive assigned KiChjang Jan 22, 2018

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

bors-servo pushed a commit that referenced this pull request Jan 22, 2018

Auto merge of #19838 - servo:rm-sequence-writer-as-it-was, r=<try>

2e32dbf

Make ToCss' SequenceWriter not monomorphise like a maniac anymore

highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 22, 2018

nox force-pushed the rm-sequence-writer-as-it-was branch from a0cf705 to 4743dc3 Compare January 22, 2018 19:35

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 22, 2018

emilio approved these changes Jan 22, 2018

View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 22, 2018

nox force-pushed the rm-sequence-writer-as-it-was branch from 4743dc3 to bd7e68d Compare January 22, 2018 20:07

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 22, 2018

nox force-pushed the rm-sequence-writer-as-it-was branch from bd7e68d to 0bf70dc Compare January 22, 2018 20:58

highfive added the S-needs-rebase There are merge conflict errors. label Jan 22, 2018

nox added 3 commits January 23, 2018 09:25

Use Image::to_css in <Image as Debug>::fmt

6d2018a

Derive ToCss for font_feature_values_rule::SingleValue

3672856

Change ToCss to take a CssWriter<W>

cd8f96c

This more concrete wrapper type can write a prefix the very first time something is written to it. This allows removing plenty of useless monomorphisations caused by the former W/SequenceWriter<W> pair of types.

highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 23, 2018

Derive ToCss for TransformOperation

42c8dc9

Now that SequenceWriter<W> does not monomorphise excessively, we can actually type check a derived ToCss without too much type recursion.

nox force-pushed the rm-sequence-writer-as-it-was branch from 04e7cce to 42c8dc9 Compare January 23, 2018 10:02

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 23, 2018

highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 23, 2018

emilio approved these changes Jan 23, 2018

View reviewed changes

highfive assigned emilio and unassigned KiChjang Jan 23, 2018

bors-servo merged commit 42c8dc9 into master Jan 23, 2018

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

This was referenced Jan 23, 2018

style: Move mask-size outside of mako #19470

Closed

Implement GridTemplateAreas with reference counting #19465

Merged

nox deleted the rm-sequence-writer-as-it-was branch January 24, 2018 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ToCss' SequenceWriter not monomorphise like a maniac anymore #19838

Make ToCss' SequenceWriter not monomorphise like a maniac anymore #19838

nox commented Jan 22, 2018 •

edited by SimonSapin

highfive commented Jan 22, 2018

highfive commented Jan 22, 2018

nox commented Jan 22, 2018

nox commented Jan 22, 2018

bors-servo commented Jan 22, 2018

bors-servo commented Jan 22, 2018

nox commented Jan 22, 2018

bors-servo commented Jan 22, 2018

emilio left a comment

emilio Jan 22, 2018

emilio Jan 22, 2018

emilio Jan 22, 2018

emilio Jan 22, 2018

emilio Jan 22, 2018

bors-servo commented Jan 22, 2018

nox commented Jan 22, 2018

bors-servo commented Jan 22, 2018

bors-servo commented Jan 22, 2018

bors-servo commented Jan 22, 2018

nox commented Jan 23, 2018

bors-servo commented Jan 23, 2018

nox commented Jan 23, 2018

bors-servo commented Jan 23, 2018

bors-servo commented Jan 23, 2018

emilio commented Jan 23, 2018

bors-servo commented Jan 23, 2018

nox commented Jan 23, 2018

bors-servo commented Jan 23, 2018

bors-servo commented Jan 23, 2018

ionutgoldan commented Jan 25, 2018

Make ToCss' SequenceWriter not monomorphise like a maniac anymore #19838

Make ToCss' SequenceWriter not monomorphise like a maniac anymore #19838

Conversation

nox commented Jan 22, 2018 • edited by SimonSapin

highfive commented Jan 22, 2018

highfive commented Jan 22, 2018

nox commented Jan 22, 2018

nox commented Jan 22, 2018

bors-servo commented Jan 22, 2018

bors-servo commented Jan 22, 2018

nox commented Jan 22, 2018

bors-servo commented Jan 22, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio Jan 22, 2018

Choose a reason for hiding this comment

emilio Jan 22, 2018

Choose a reason for hiding this comment

emilio Jan 22, 2018

Choose a reason for hiding this comment

emilio Jan 22, 2018

Choose a reason for hiding this comment

emilio Jan 22, 2018

Choose a reason for hiding this comment

bors-servo commented Jan 22, 2018

nox commented Jan 22, 2018

bors-servo commented Jan 22, 2018

bors-servo commented Jan 22, 2018

bors-servo commented Jan 22, 2018

nox commented Jan 23, 2018

bors-servo commented Jan 23, 2018

nox commented Jan 23, 2018

bors-servo commented Jan 23, 2018

bors-servo commented Jan 23, 2018

emilio commented Jan 23, 2018

bors-servo commented Jan 23, 2018

nox commented Jan 23, 2018

bors-servo commented Jan 23, 2018

bors-servo commented Jan 23, 2018

ionutgoldan commented Jan 25, 2018

nox commented Jan 22, 2018 •

edited by SimonSapin