Closed
Bug 1443046
Opened 7 years ago
Closed 7 years ago
stylo: URLValueData should be shared between specified value and computed value
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
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 | ||
Updated•7 years ago
|
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbca3f092944ed4aea6826157b4da240d918d3d9
The test_value_storage.html failures have been addressed in the updated version.
Comment 13•7 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
Attachment #8957050 -
Flags: review?(emilio) → review+
Comment 14•7 years ago
|
||
mozreview-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 15•7 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) {
Attachment #8957051 -
Flags: review?(emilio) → review+
Comment 16•7 years ago
|
||
mozreview-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 17•7 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
Attachment #8957054 -
Flags: review?(emilio) → review+
Comment 18•7 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? :)
Attachment #8957055 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 19•7 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.
Comment 20•7 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!
Assignee | ||
Comment 21•7 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8957050 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8957051 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8957052 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8957053 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8957054 -
Attachment is obsolete: true
Comment 28•7 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
Comment 29•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 30•7 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
You need to log in
before you can comment on or make changes to this bug.
Description
•