1431268 - stylo: Avoid separate monomorphizations of CSS serialization for utf-8 and utf-16

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Bobby Holley (:bholley)]

We currently have some accidental usage of utf-8 serialization for stylo, which is primarily utf-16. We could just fix those, but this is pretty easy to screw up, so I'm going to make the relevant routines non-generic to avoid this happening again.

With this patch, we only have a single monomorphization of PropertyDeclaration::to_css.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Avoid some unnecessary intermediate utf8 strings in glue.rs. v1

MozReview-Commit-ID: 79qv87uPzjR

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Avoid the generic writer parameter for PropertyDeclaration serialization. v1

MozReview-Commit-ID: JR3IcL1NRHO

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8943453 [details] [diff] [review]
Part 1 - Avoid some unnecessary intermediate utf8 strings in glue.rs. v1

Review of attachment 8943453 [details] [diff] [review]:
-----------------------------------------------------------------

Fascinating.

r=me, thanks!

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8943454 [details] [diff] [review]
Part 2 - Avoid the generic writer parameter for PropertyDeclaration serialization. v1

Review of attachment 8943454 [details] [diff] [review]:
-----------------------------------------------------------------

Fix the nits, then r=me

::: servo/components/style/gecko/rules.rs
@@ +206,1 @@
>          let mut css_text = nsString::new();

This could just pass CssWriter instead of doing nsString::new(), then calling into FFI, then write!, right?

I guess you'd need to make sure that GetCssText only appends and not truncates.

@@ +242,1 @@
>          let mut css_text = nsString::new();

ditto (not sure if worth fixing, just pointing it out)

::: servo/components/style/properties/declaration_block.rs
@@ +306,5 @@
>  
>      /// Find the value of the given property in this block and serialize it
>      ///
>      /// <https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue>
> +    pub fn property_value_to_css(&self, property: &PropertyId, dest: &mut CssWriter) -> fmt::Result

nit: brace to the previous line, or block-indent.

@@ +615,3 @@
>          computed_values: Option<&ComputedValues>,
>          custom_properties_block: Option<&PropertyDeclarationBlock>,
>      ) -> fmt::Result

nit: brace to the previous line.

@@ +741,5 @@
> +    /// accidentally monomorphizing this large function multiple times for
> +    /// different writers.
> +    ///
> +    /// https://drafts.csswg.org/cssom/#serialize-a-css-declaration-block
> +    pub fn to_css(&self, dest: &mut CssWriter) -> fmt::Result

nit: brace.

@@ +966,5 @@
>      }
>  }
>  
>  /// Append a given kind of appendable value to a serialization.
> +pub fn append_declaration_value<'a, I>(dest: &mut CssWriter,

nit: If you want to indent this using block indentation while you're touching it, it'd be wonderful.

@@ +986,5 @@
>  }
>  
>  /// Append a given property and value pair to a serialization.
> +pub fn append_serialization<'a, I, N>(dest: &mut CssWriter,
> +                                      property_name: &N,

ditto.

::: servo/components/style/shared_lock.rs
@@ +219,5 @@
>      impl<'a> Marker2 for SharedRwLockReadGuard<'a> {}  // Assert SharedRwLockReadGuard: !Copy
>      impl<'a> Marker2 for SharedRwLockWriteGuard<'a> {}  // Assert SharedRwLockWriteGuard: !Copy
>  }
>  
> +/// Like ToCssConcrete, but with a lock guard given by the caller.

nit: There's no ToCssConcrete, please fix the comment.

@@ +228,5 @@
>      /// Serialize `self` in CSS syntax using the given lock guard and return a string.
>      ///
>      /// (This is a convenience wrapper for `to_css` and probably should not be overridden.)
>      #[inline]
> +    fn to_css_string(&self, guard: &SharedRwLockReadGuard) -> CssString {

Is this actually used? If not, you may as well remove it. It was there for symmetry with ToCss, but that's no longer the case anyway.

::: servo/components/style/str.rs
@@ +192,5 @@
> +impl<'a> CssStringBorrow<'a> {
> +    /// Writes the borrowed string to the provided writer.
> +    pub fn append_to(&self, dest: &mut CssWriter) -> fmt::Result {
> +        match *self {
> +            CssStringBorrow::UTF16(s) => { dest.append(s); Ok(()) },

nit: newline after brace?

@@ +240,5 @@
> +        dest.write_str(self.0)
> +    }
> +
> +    /// Returns true if the borrowed string is empty.
> +    pub fn is_empty(&self) -> bool { self.0.is_empty() }

nit: ditto

@@ +250,5 @@
> +}
> +
> +#[cfg(feature = "servo")]
> +impl<'a> From<&'a String> for CssStringBorrow<'a> {
> +    fn from(s: &'a String) -> Self { CssStringBorrow(&*s) }

ditto

::: servo/components/style/stylesheets/font_feature_values_rule.rs
@@ +347,5 @@
>              }
>          }
>  
>          impl ToCssWithGuard for FontFeatureValuesRule {
> +            fn to_css(&self, _guard: &SharedRwLockReadGuard, dest: &mut CssWriter) -> fmt::Result

nit: brace

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(Sorry for the lag between reviews)

Comment 7

[Anthony Ramine [:nox]]

Please rename CssWriter, I'm actually going to need to change ToCss to take a CssWriter<W> to avoid the SequenceWriter monomorphisations.

Comment 8

[Bobby Holley (:bholley)]

Per comment 7 and IRC discussion, I'm renaming CssWriter to CssStringWriter.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Comment on attachment 8943454 [details] [diff] [review]
> Part 2 - Avoid the generic writer parameter for PropertyDeclaration
> serialization. v1
> 
> Review of attachment 8943454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fix the nits, then r=me
> 
> ::: servo/components/style/gecko/rules.rs
> @@ +206,1 @@
> >          let mut css_text = nsString::new();
> 
> This could just pass CssWriter instead of doing nsString::new(), then
> calling into FFI, then write!, right?
> 
> I guess you'd need to make sure that GetCssText only appends and not
> truncates.

Yeah it does Assign, unfortunately: https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/layout/style/nsCSSFontFaceRule.cpp#398
> 
> @@ +242,1 @@
> >          let mut css_text = nsString::new();
> 
> ditto (not sure if worth fixing, just pointing it out)

Same.

> 
> ::: servo/components/style/properties/declaration_block.rs
> @@ +306,5 @@
> >  
> >      /// Find the value of the given property in this block and serialize it
> >      ///
> >      /// <https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue>
> > +    pub fn property_value_to_css(&self, property: &PropertyId, dest: &mut CssWriter) -> fmt::Result
> 
> nit: brace to the previous line, or block-indent.

Fixed.

> 
> @@ +615,3 @@
> >          computed_values: Option<&ComputedValues>,
> >          custom_properties_block: Option<&PropertyDeclarationBlock>,
> >      ) -> fmt::Result
> 
> nit: brace to the previous line.

Fixed.

> 
> @@ +741,5 @@
> > +    /// accidentally monomorphizing this large function multiple times for
> > +    /// different writers.
> > +    ///
> > +    /// https://drafts.csswg.org/cssom/#serialize-a-css-declaration-block
> > +    pub fn to_css(&self, dest: &mut CssWriter) -> fmt::Result
> 
> nit: brace.

Fixed.

> 
> @@ +966,5 @@
> >      }
> >  }
> >  
> >  /// Append a given kind of appendable value to a serialization.
> > +pub fn append_declaration_value<'a, I>(dest: &mut CssWriter,
> 
> nit: If you want to indent this using block indentation while you're
> touching it, it'd be wonderful.

Fixed.

> 
> @@ +986,5 @@
> >  }
> >  
> >  /// Append a given property and value pair to a serialization.
> > +pub fn append_serialization<'a, I, N>(dest: &mut CssWriter,
> > +                                      property_name: &N,
> 
> ditto.

Fixed.

> 
> ::: servo/components/style/shared_lock.rs
> @@ +219,5 @@
> >      impl<'a> Marker2 for SharedRwLockReadGuard<'a> {}  // Assert SharedRwLockReadGuard: !Copy
> >      impl<'a> Marker2 for SharedRwLockWriteGuard<'a> {}  // Assert SharedRwLockWriteGuard: !Copy
> >  }
> >  
> > +/// Like ToCssConcrete, but with a lock guard given by the caller.
> 
> nit: There's no ToCssConcrete, please fix the comment.

Fixed.

> 
> @@ +228,5 @@
> >      /// Serialize `self` in CSS syntax using the given lock guard and return a string.
> >      ///
> >      /// (This is a convenience wrapper for `to_css` and probably should not be overridden.)
> >      #[inline]
> > +    fn to_css_string(&self, guard: &SharedRwLockReadGuard) -> CssString {
> 
> Is this actually used? If not, you may as well remove it. It was there for
> symmetry with ToCss, but that's no longer the case anyway.

It's used all over the script crate.

> 
> ::: servo/components/style/str.rs
> @@ +192,5 @@
> > +impl<'a> CssStringBorrow<'a> {
> > +    /// Writes the borrowed string to the provided writer.
> > +    pub fn append_to(&self, dest: &mut CssWriter) -> fmt::Result {
> > +        match *self {
> > +            CssStringBorrow::UTF16(s) => { dest.append(s); Ok(()) },
> 
> nit: newline after brace?

fixed.

> 
> @@ +240,5 @@
> > +        dest.write_str(self.0)
> > +    }
> > +
> > +    /// Returns true if the borrowed string is empty.
> > +    pub fn is_empty(&self) -> bool { self.0.is_empty() }
> 
> nit: ditto

Fixed.

> 
> @@ +250,5 @@
> > +}
> > +
> > +#[cfg(feature = "servo")]
> > +impl<'a> From<&'a String> for CssStringBorrow<'a> {
> > +    fn from(s: &'a String) -> Self { CssStringBorrow(&*s) }
> 
> ditto

Fixed.

> 
> ::: servo/components/style/stylesheets/font_feature_values_rule.rs
> @@ +347,5 @@
> >              }
> >          }
> >  
> >          impl ToCssWithGuard for FontFeatureValuesRule {
> > +            fn to_css(&self, _guard: &SharedRwLockReadGuard, dest: &mut CssWriter) -> fmt::Result
> 
> nit: brace

Fixed.

Comment 9

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/19841

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/mozilla-central/rev/073c7a9e4309

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8943454 [details] [diff] [review]
Part 2 - Avoid the generic writer parameter for PropertyDeclaration serialization. v1

I think we should uplift this, along with https://hg.mozilla.org/integration/autoland/rev/90b986308ba8 , which together reduce installer size by ~200k. We can let them bake for a few days on nightly.

Approval Request Comment
[Feature/Bug causing the regression]: Stylo (so not a regression in 59 per se)
[User impact if declined]: Larger download size
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just some refactoring of code to allow the compiler to generate a smaller binary.
[String changes made/needed]: None

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Based on comment 11, I'll assume we will give this some time in nightly and consider uplift for beta 5 next week rather than tomorrow's build.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Based on comment 11, I'll assume we will give this some time in nightly and
> consider uplift for beta 5 next week rather than tomorrow's build.

Yeah sounds good to me. Thanks.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8943454 [details] [diff] [review]
Part 2 - Avoid the generic writer parameter for PropertyDeclaration serialization. v1

Smaller download size sounds like a good thing. Let's uplift this for 59 beta 6.

Comment 15

[Bobby Holley (:bholley)]

Note: There's a patch between these two that will cause build issues on a naive cherry-pick. I'll handle the uplift.

Comment 16

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/e3a762c0966d4f69d047cca8b1e626bbed93cabd
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/906fd0e933272fba3af045e70f98a90ef2366edc

Comment 17

[Ionuț Goldan [:igoldan]]

This improved our build times for beta:

== Change summary for alert #11388 (as of Tue, 30 Jan 2018 20:52:21 GMT) ==

Improvements:

 13%  build times windows2012-64 opt static-analysis taskcluster-c4.4xlarge     2,735.96 -> 2,386.36
 12%  build times windows2012-32-add-on-devel opt taskcluster-c4.4xlarge        2,922.25 -> 2,583.39
 12%  build times windows2012-64-add-on-devel opt taskcluster-c4.4xlarge        3,049.11 -> 2,696.40
 11%  build times windows2012-32 opt static-analysis taskcluster-c4.4xlarge     2,433.98 -> 2,160.67
  6%  build times windows2012-32 opt nightly taskcluster-c4.4xlarge             4,529.93 -> 4,266.18
  6%  build times windows2012-64 opt taskcluster-c4.4xlarge                     4,843.60 -> 4,570.54
  6%  build times windows2012-32 opt taskcluster-c4.4xlarge                     4,526.94 -> 4,272.36
  6%  build times windows2012-64-devedition opt nightly taskcluster-c4.4xlarge  4,895.29 -> 4,621.32
  6%  build times windows2012-32-devedition opt nightly taskcluster-c4.4xlarge  4,538.19 -> 4,287.53
  6%  build times windows2012-64 opt nightly taskcluster-c4.4xlarge             4,865.12 -> 4,596.97

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