Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 6 years ago

We are not sharing URLValueData between specified value and computed value, [1] which makes us have to resolve url for each element rather than each used rule.

It doesn't affect images because we have been able to share image values, but this issue probably affects everything using URL outside images. That includes -moz-binding and SVG things like filter, and marker-{start,mid,end}.

This takes a significant time in tpaint (2.4ms in PresShell::Initialize, and probably some more after that).


[1] https://github.com/servo/servo/blob/e3f69668aefaaac36e59509bd978dfda05018486/components/style/properties/gecko.mako.rs#L942-L949

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 12 • 6 years ago

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

The test_value_storage.html failures have been addressed in the updated version.

	Emilio Cobos Álvarez (:emilio)
	Comment 13 • 6 years ago
mozreview-review

Comment on attachment 8957050 [details]
Remove some useless clone() call on SpecifiedUrl::image_value.

https://reviewboard.mozilla.org/r/226014/#review231924

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8957053 [details]
Move ComputedUrl into their impl mods.

https://reviewboard.mozilla.org/r/226020/#review231928

	Emilio Cobos Álvarez (:emilio)
	Comment 15 • 6 years ago
mozreview-review

Comment on attachment 8957051 [details]
Add SpecifiedImageUrl for <url> used as images.

https://reviewboard.mozilla.org/r/226016/#review231926

r=me with the different nits addressed as you see fit.

::: servo/components/style/gecko/url.rs:118
(Diff revision 2)
> +        0
> +    }
> +}
> +
> +/// A specified url() value for image.
> +#[derive(Clone, Debug)]

Derive ToCss, using `#[css(skip)]` on the image value.

Please add a comment about why this exists and so on.

::: servo/components/style/gecko/url.rs:139
(Diff revision 2)
> -            }
> +    }
> -        }
> +}
> +
> +impl SpecifiedImageUrl {
> +    /// Parse a URL from a string value. See SpecifiedUrl::parse_from_string.
> +    pub fn parse_from_string<'a>(

This looks unused? (though wont if you address one of the comments below)

::: servo/components/style/gecko/url.rs:153
(Diff revision 2)
> +        if image_request.mImageValue.mRawPtr.is_null() {
> +            return Err(());
> -}
> +        }
>  
> -impl MallocSizeOf for SpecifiedUrl {
> -    fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
> +        let image_value = image_request.mImageValue.mRawPtr.as_ref().unwrap();
> +        let ref url_value_data = image_value._base;

I don't think we have many occurrences of `let ref`, maybe use `let url_value_data = &image_value._base;`?

I guess you're just moving code around, so not a big deal.

::: servo/components/style/gecko/url.rs:168
(Diff revision 2)
> +    }
> +}
>  
> -        // XXX: measure `serialization` once bug 1397971 lands
> +impl Eq for SpecifiedImageUrl {}
>  
> -        // We ignore `extra_data`, because RefPtr is tricky, and there aren't
> +impl From<SpecifiedUrl> for SpecifiedImageUrl {

So, not a fan of `From` impls that do anything so non-trivial... I think that using SpecifiedImageUrl in a few places below should make this not needed, what do you think?

Though I guess you maybe need to add another `fn from_url_value_data`...

::: servo/components/style/values/computed/mod.rs:653
(Diff revision 2)
>  
> -/// TODO: Properly build ComputedUrl for gecko
> +/// The computed value of a CSS `url()` for image.
> +#[cfg(feature = "servo")]
> +pub type ComputedImageUrl = ComputedUrl;
> +
> +// TODO: Properly build ComputedUrl for gecko

I don't think this `TODO` has any value, tbh.

::: servo/components/style/values/specified/image.rs:938
(Diff revision 2)
>  impl Parse for MozImageRect {
>      fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
>          input.try(|i| i.expect_function_matching("-moz-image-rect"))?;
>          input.parse_nested_block(|i| {
>              let string = i.expect_url_or_string()?;
>              let url = SpecifiedUrl::parse_from_string(string.as_ref().to_owned(), context)?;

Use the `SpecifiedImageUrl`?

That way you can use hte shorthand syntax below:

```
MozImageRect { url, top, right, ... }
```

::: servo/components/style/values/specified/list.rs:97
(Diff revision 2)
>  impl Parse for ListStyleImage {
> -    fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
> -                         -> Result<ListStyleImage, ParseError<'i>> {
> -        #[allow(unused_mut)]
> -        let mut value = input.try(|input| UrlOrNone::parse(context, input))?;
> -
> +    fn parse<'i, 't>(
> +        context: &ParserContext,
> +        input: &mut Parser<'i, 't>,
> +    ) -> Result<ListStyleImage, ParseError<'i>> {
> +        input.try(|input| ImageUrlOrNone::parse(context, input)).map(ListStyleImage)

While you're here, there's no need to `input.try` this, just `ImageUrlOrNone::parse` should do.

::: servo/ports/geckolib/glue.rs:3558
(Diff revision 2)
>          url_data,
>          Some(CssRuleType::Style),
>          ParsingMode::DEFAULT,
>          QuirksMode::NoQuirks,
>      );
> -    if let Ok(mut url) = SpecifiedUrl::parse_from_string(string.into(), &context) {
> +    if let Ok(url) = SpecifiedUrl::parse_from_string(string.into(), &context) {

You can use `SpecifiedImageUrl` directly right?

::: servo/ports/geckolib/glue.rs:3558
(Diff revision 2)
>          url_data,
>          Some(CssRuleType::Style),
>          ParsingMode::DEFAULT,
>          QuirksMode::NoQuirks,
>      );
> -    if let Ok(mut url) = SpecifiedUrl::parse_from_string(string.into(), &context) {
> +    if let Ok(url) = SpecifiedUrl::parse_from_string(string.into(), &context) {

	Emilio Cobos Álvarez (:emilio)
	Comment 16 • 6 years ago
mozreview-review

Comment on attachment 8957052 [details]
Move Parse impl of SpecifiedUrl into corresponding files.

https://reviewboard.mozilla.org/r/226018/#review231932

	Emilio Cobos Álvarez (:emilio)
	Comment 17 • 6 years ago
mozreview-review

Comment on attachment 8957054 [details]
Split CssUrl from SpecifiedUrl for non-value URLs.

https://reviewboard.mozilla.org/r/226022/#review231930

So I'm not convinced CssUrl is the right name given it still has a to_computed_value method in servo and all that... but maybe that's fine.

::: servo/components/style/gecko/url.rs:160
(Diff revision 2)
> +    fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
> +        CssUrl::parse(context, input).map(From::from)
> +    }
> +}
> +
> +impl ToCss for SpecifiedUrl {

Derive

	Emilio Cobos Álvarez (:emilio)
	Comment 18 • 6 years ago
mozreview-review

Comment on attachment 8957055 [details]
Bug 1443046 - Construct URLValue eagerly and share it between specified value and style structs.

https://reviewboard.mozilla.org/r/226024/#review231934

r=me, though I'd measure whether this affects parse times / memory and such. Probably not much, given we're doing this for images anyway?

::: servo/components/style/gecko/url.rs:144
(Diff revision 2)
>      }
> +}
>  
> -    /// Create a bundled URI suitable for sending to Gecko
> -    /// to be constructed into a css::URLValue.
> -    ///
> +fn build_url_value(url: &CssUrl) -> RefPtr<URLValue> {
> +    unsafe {
> +        let ptr = bindings::Gecko_NewURLValue(url.for_ffi());

Heh, I guess now in practice we're resolving all URIs eagerly, which means that the comment about Servo's setup pointing to https://bugzilla.mozilla.org/show_bug.cgi?id=1347435#c6 can probably go away? :)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 19 • 6 years ago
mozreview-review-reply

Comment on attachment 8957055 [details]
Bug 1443046 - Construct URLValue eagerly and share it between specified value and style structs.

https://reviewboard.mozilla.org/r/226024/#review231934

> Heh, I guess now in practice we're resolving all URIs eagerly, which means that the comment about Servo's setup pointing to https://bugzilla.mozilla.org/show_bug.cgi?id=1347435#c6 can probably go away? :)

I don't think so. URLValue in this case would simply carry the values without actually resolving it, because resolving a URLValue requires main thread.

	Emilio Cobos Álvarez (:emilio)
	Comment 20 • 6 years ago
mozreview-review-reply

Comment on attachment 8957055 [details]
Bug 1443046 - Construct URLValue eagerly and share it between specified value and style structs.

https://reviewboard.mozilla.org/r/226024/#review231934

> I don't think so. URLValue in this case would simply carry the values without actually resolving it, because resolving a URLValue requires main thread.

Oh, that's right, I always forget about this weird setup. Anyway, thanks for fixing this!

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 21 • 6 years ago
mozreview-review-reply

Comment on attachment 8957051 [details]
Add SpecifiedImageUrl for <url> used as images.

https://reviewboard.mozilla.org/r/226016/#review231926

> I don't think this `TODO` has any value, tbh.

This TODO is pre-existing... so I'm not going to touch it for now... at least in this commit.

	Pulsebot
	Comment 28 • 6 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78cff5f101db
Construct URLValue eagerly and share it between specified value and style structs. r=emilio

	Cristian Brindusan [:cbrindusan]
	Comment 29 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/78cff5f101db

	Ionuț Goldan [:igoldan]
	Comment 30 • 6 years ago

Perf improvements wins:

== Change summary for alert #12030 (as of Thu, 08 Mar 2018 12:45:00 GMT) ==

Improvements:

  2%  about_preferences_basic osx-10-10 opt e10s stylo     212.95 -> 207.94
  2%  about_preferences_basic linux64 opt e10s stylo       147.49 -> 144.94

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