Closed Bug 1443046 Opened 2 years ago Closed 2 years ago

stylo: URLValueData should be shared between specified value and computed value

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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
Assignee: nobody → xidorn+moz
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbca3f092944ed4aea6826157b4da240d918d3d9

The test_value_storage.html failures have been addressed in the updated version.
Comment on attachment 8957050 [details]
Remove some useless clone() call on SpecifiedUrl::image_value.

https://reviewboard.mozilla.org/r/226014/#review231924
Attachment #8957050 - Flags: review?(emilio) → review+
Comment on attachment 8957053 [details]
Move ComputedUrl into their impl mods.

https://reviewboard.mozilla.org/r/226020/#review231928
Attachment #8957053 - Flags: review?(emilio) → 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) {
Attachment #8957051 - Flags: review?(emilio) → review+
Comment on attachment 8957052 [details]
Move Parse impl of SpecifiedUrl into corresponding files.

https://reviewboard.mozilla.org/r/226018/#review231932
Attachment #8957052 - Flags: review?(emilio) → 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
Attachment #8957054 - Flags: review?(emilio) → 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? :)
Attachment #8957055 - Flags: review?(emilio) → 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

> 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.
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!
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.
Attachment #8957050 - Attachment is obsolete: true
Attachment #8957051 - Attachment is obsolete: true
Attachment #8957052 - Attachment is obsolete: true
Attachment #8957053 - Attachment is obsolete: true
Attachment #8957054 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/78cff5f101db
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
You need to log in before you can comment on or make changes to this bug.