1397971 - stylo: lots of memory used for SpecifiedURLs relating to images for gmail

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

Some DMD output on Gmail:

> Unreported {
>   11,193 blocks in heap block record 1 of 14,297
>   2,865,408 bytes (2,865,408 requested / 0 slop)
>   Individual block sizes: 256 x 11,193
>   1.78% of the heap (1.78% cumulative)
>   10.30% of unreported (10.30% cumulative)
>   Allocated at {
>     #01: replace_malloc (/home/njn/moz/autoland/memory/replace/dmd/DMD.cpp:1303 (discriminator 2))
>     #02: nsStringBuffer::Alloc(unsigned long) (/home/njn/moz/autoland/xpcom/string/nsSubstring.cpp:238)
>     #03: nsAString::MutatePrep(unsigned int, char16_t**, mozilla::detail::StringDataFlags*) (/home/njn/moz/autoland/o64sty/xpcom/string/../../../xpcom/string/nsTSubstring.cpp:160 (discriminator 1))
>     #04: nsAString::SetCapacity(unsigned int, mozilla::fallible_t const&) (/home/njn/moz/autoland/o64sty/xpcom/string/../../../xpcom/string/nsTSubstring.cpp:692)
>     #05: nsAString::SetLength(unsigned int, mozilla::fallible_t const&) (/home/njn/moz/autoland/o64sty/xpcom/string/../../../xpcom/string/nsTSubstring.cpp:730)
>     #06: AppendUTF8toUTF16(nsACString const&, nsAString&, mozilla::fallible_t const&) (/home/njn/moz/autoland/xpcom/string/nsReadableUtils.cpp:365)
>     #07: AppendUTF8toUTF16(nsACString const&, nsAString&) (/home/njn/moz/autoland/xpcom/string/nsReadableUtils.cpp:344)
>     #08: NS_ConvertUTF8toUTF16 (/home/njn/moz/autoland/o64sty/storage/../dist/include/nsString.h:138 (discriminator 2))
>     #09: operator new(unsigned long) (/home/njn/moz/autoland/o64sty/layout/style/../../dist/include/mozilla/mozalloc.h:206)
>     #10: style::gecko::url::{{impl}}::build_image_value (/home/njn/moz/autoland/servo/components/style/gecko/url.rs:116)
>     #11: style::values::specified::image::{{impl}}::parse (/home/njn/moz/autoland/servo/components/style/values/specified/image.rs:138)
>     #12: style::values::{{impl}}::parse<style::values::None_,style::values::generics::image::Image<style::values::generics::image::Gradient<style::values::specified::image::LineDirection, style::values::specified::length::Length, style::values::specified::length::LengthOrPercentage, style::values::specified::image::GradientPosition, style::values::specified::color::RGBAColor, style::values::specified::angle::Angle>, style::values::generics::image::MozImageRect<style::values::specified::NumberOrPercentage, style::gecko::url::SpecifiedUrl>, style::gecko::url::SpecifiedUrl>> (/home/njn/moz/autoland/servo/components/style/values/mod.rs:95)
>     #13: style::properties::longhands::background_image::single_value::parse (/home/njn/moz/autoland/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-e4d17c1a5eeb1576/out/properties.rs:358)
>     #14: style::properties::shorthands::background::parse_value::{{closure}}::{{closure}} (/home/njn/moz/autoland/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-e4d17c1a5eeb1576/out/properties.rs:50502)
>     #15: cssparser::parser::{{impl}}::try<closure,style::values::Either<style::values::None_, style::values::generics::image::Image<style::values::generics::image::Gradient<style::values::specified::image::LineDirection, style::values::specified::length::Length, style::values::specified::length::LengthOrPercentage, style::values::specified::image::GradientPosition, style::values::specified::color::RGBAColor, style::values::specified::angle::Angle>, style::values::generics::image::MozImageRect<style::values::specified::NumberOrPercentage, style::gecko::url::SpecifiedUrl>, style::gecko::url::SpecifiedUrl>>,cssparser::parser::ParseError<selectors::parser::SelectorParseError<style_traits::StyleParseError>>> (/home/njn/moz/autoland/third_party/rust/cssparser/src/parser.rs:365)
>     #16: style::properties::shorthands::background::parse_value::{{closure}} (/home/njn/moz/autoland/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-e4d17c1a5eeb1576/out/properties.rs:50501)
>   }
> }
> 
> Unreported {
>   11,497 blocks in heap block record 4 of 14,297
>   1,287,664 bytes (1,195,688 requested / 91,976 slop)
>   Individual block sizes: 112 x 11,497
>   0.80% of the heap (4.68% cumulative)
>   4.63% of unreported (27.01% cumulative)
>   Allocated at {
>     #01: replace_malloc (/home/njn/moz/autoland/memory/replace/dmd/DMD.cpp:1303 (discriminator 2))
>     #02: moz_xmalloc (/home/njn/moz/autoland/memory/mozalloc/mozalloc.cpp:85 (discriminator 2))
>     #03: operator new(unsigned long) (/home/njn/moz/autoland/o64sty/layout/style/../../dist/include/mozilla/mozalloc.h:206)
>     #04: style::gecko::url::{{impl}}::build_image_value (/home/njn/moz/autoland/servo/components/style/gecko/url.rs:116)
>     #05: style::values::specified::image::{{impl}}::parse (/home/njn/moz/autoland/servo/components/style/values/specified/image.rs:138)
>     #06: style::values::{{impl}}::parse<style::values::None_,style::values::generics::image::Image<style::values::generics::image::Gradient<style::values::specified::image::LineDirection, style::values::specified::length::Length, style::values::specified::length::LengthOrPercentage, style::values::specified::image::GradientPosition, style::values::specified::color::RGBAColor, style::values::specified::angle::Angle>, style::values::generics::image::MozImageRect<style::values::specified::NumberOrPercentage, style::gecko::url::SpecifiedUrl>, style::gecko::url::SpecifiedUrl>> (/home/njn/moz/autoland/servo/components/style/values/mod.rs:95)
>     #07: style::properties::longhands::background_image::single_value::parse (/home/njn/moz/autoland/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-e4d17c1a5eeb1576/out/properties.rs:358)
>     #08: style::properties::shorthands::background::parse_value::{{closure}}::{{closure}} (/home/njn/moz/autoland/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-e4d17c1a5eeb1576/out/properties.rs:50502)
>     #09: cssparser::parser::{{impl}}::try<closure,style::values::Either<style::values::None_, style::values::generics::image::Image<style::values::generics::image::Gradient<style::values::specified::image::LineDirection, style::values::specified::length::Length, style::values::specified::length::LengthOrPercentage, style::values::specified::image::GradientPosition, style::values::specified::color::RGBAColor, style::values::specified::angle::Angle>, style::values::generics::image::MozImageRect<style::values::specified::NumberOrPercentage, style::gecko::url::SpecifiedUrl>, style::gecko::url::SpecifiedUrl>>,cssparser::parser::ParseError<selectors::parser::SelectorParseError<style_traits::StyleParseError>>> (/home/njn/moz/autoland/third_party/rust/cssparser/src/parser.rs:365)
>     #10: style::properties::shorthands::background::parse_value::{{closure}} (/home/njn/moz/autoland/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-e4d17c1a5eeb1576/out/properties.rs:50501)
>     #11: core::ops::impls::{{impl}}::call_once<(&mut cssparser::parser::Parser),closure> (/checkout/src/libcore/ops.rs:2732)
>     #12: cssparser::parser::Parser::parse_entirely (style.cgu-0.rs:?)
>     #13: cssparser::parser::parse_until_before<&mut closure,(),selectors::parser::SelectorParseError<style_traits::StyleParseError>> (/home/njn/moz/autoland/third_party/rust/cssparser/src/parser.rs:785)
>     #14: cssparser::parser::{{impl}}::parse_until_before<&mut closure,(),selectors::parser::SelectorParseError<style_traits::StyleParseError>> (/home/njn/moz/autoland/third_party/rust/cssparser/src/parser.rs:523)
>     #15: cssparser::parser::{{impl}}::parse_comma_separated<closure,(),selectors::parser::SelectorParseError<style_traits::StyleParseError>> (/home/njn/moz/autoland/third_party/rust/cssparser/src/parser.rs:484)
>     #16: style::properties::shorthands::background::parse_value (/home/njn/moz/autoland/o64sty/dist/bin/libxul.so)
>   }
> }

That's 4 MiB just for URL strings that are being created by
Gecko_ImageValue_Create(), which convert utf8 URL strings to utf16.

I added some printfs and got the following URLs:

> 12194 counts:
> (  1)     4592 (37.7%, 37.7%): ImageValue: //ssl.gstatic.com/chat/emoji/png28-7f9d3a5045813584f828fe69a1fecb77.png 71
> (  2)     1148 ( 9.4%, 47.1%): ImageValue: //ssl.gstatic.com/chat/emoji/highdpi1-393a0a2bf2825ecf8f028e4eda2fdd22.png 74
> (  3)     1148 ( 9.4%, 56.5%): ImageValue: //ssl.gstatic.com/chat/emoji/highdpi3-7422e1a9cf9f8f03943f945c68c11fcc.png 74
> (  4)     1148 ( 9.4%, 65.9%): ImageValue: //ssl.gstatic.com/chat/emoji/highdpi2-ce1a58ad2e972be0b0887b4ee4fb3184.png 74
> (  5)     1144 ( 9.4%, 75.3%): ImageValue: //ssl.gstatic.com/chat/emoji/highdpi4-15e8921be4d0e7ac815657f01fdee460.png 74
> (  6)      759 ( 6.2%, 81.5%): ImageValue: //ssl.gstatic.com/chat/babble/sprites/common-517bca7f07a37c64c3263a1411e266a5.png 81
> (  7)      744 ( 6.1%, 87.6%): ImageValue: //ssl.gstatic.com/chat/babble/sprites/highdpi-1cb87125492385c4ff0cd94fb144aa0f.png 82
> (  8)       51 ( 0.4%, 88.0%): ImageValue: https://ssl.gstatic.com/cloudsearch/static/o/d/0016-a3cdcdc31a16b3497ed6ffcdaee4f325/icons.png 94
> (  9)       43 ( 0.4%, 88.4%): ImageValue: https://ssl.gstatic.com/mail/sprites/general-76906fbc82f79822625038e272fa3893.png 81
> ( 10)       43 ( 0.4%, 88.7%): ImageValue: //ssl.gstatic.com/ui/v1/icons/mail/skinnable/skinnable_ltr_light_1x.png 71
> ( 11)       39 ( 0.3%, 89.1%): ImageValue: https://ssl.gstatic.com/mail/sprites/general_2x-8f803f655d38e38581ce96b4bca05016.png 84
> ( 12)       32 ( 0.3%, 89.3%): ImageValue: ../images/beacon._TTW_.svg 26
> ( 13)       30 ( 0.2%, 89.6%): ImageValue: ../images/beacon._TTW_.png 26
> ( 14)       30 ( 0.2%, 89.8%): ImageValue: //ssl.gstatic.com/docs/picker/images/onepick_sprite12.svg 57
> ( 15)       30 ( 0.2%, 90.1%): ImageValue: //ssl.gstatic.com/ui/v1/zippy/arrow_down.png 44
> ( 16)       27 ( 0.2%, 90.3%): ImageValue: https://ssl.gstatic.com/mail/sprites/compose_2x-0d48240003ff6a0820e96c1a03f5c537.png 84
> ( 17)       27 ( 0.2%, 90.5%): ImageValue: https://ssl.gstatic.com/mail/sprites/compose-28e1860af47052bc9ce7a152b803c105.png 81
> ( 18)       26 ( 0.2%, 90.7%): ImageValue: https://ssl.gstatic.com/mail/sprites/newattachmentcards_2x-b5dc0789db8a33abf60dba93f1731809.png 95
> ( 19)       26 ( 0.2%, 90.9%): ImageValue: https://ssl.gstatic.com/mail/sprites/newattachmentcards-ff2ce2bea04dec2bf32f2ebbfa0834ff.png 92
> ( 20)       24 ( 0.2%, 91.1%): ImageValue: //ssl.gstatic.com/ui/v1/icons/mail/im/emoticons/1/s.png 55
> 

The number at the end is the length in chars; double that to get bytes.

So it's pretty ordinary URLs, just with *many* repeats. There are also some
data: URIs, some with lengths in the low thousands, but they are few enough
that even when I weight the histogram by lengths they don't matter much.

I see two possible approaches here:

- Try to avoid the utf8-to-utf16 duplication.

- Do some kind of sharing to make the repeats cheaper.

Comment 1

[Bobby Holley (:bholley)]

Simon or Josh, could one of you have a look at this?

Comment 2

[Nicholas Nethercote [inactive]]

The main use of ImageLayer::mString is that it's returned by nsCSSValue::GetOriginalURLValue(), the result of which is passed to AppendEscapedCSSString() in a couple of places.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Right, we need mString to do serialization.  I guess the rust side stores it too, so we have two copies of it being carted around?  :(

I wonder whether we could use some magic value to indicate "read it from the rust side" for the moment, until we kill it off from the C++ side entirely (requires dropping the Gecko style system).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Another option would be to make mString a tagged union of some sort, containing either an owning ref to a UTF-16 string or a non-owning ref to a UTF-8 string owned by the servo side.  That's assuming we can rely on the servo side outliving the Gecko side, of course.

Comment 5

[Josh Matthews [:jdm]]

Attachment: WIP share strings in URLDataValue with Rust

Dumping my work so far. It's enough to get gmail to run.

Comment 6

[Josh Matthews [:jdm]]

I compared the heap-unclassified value I captured as soon as possible after the main gmail inbox finished loading in my account. The value in the build with the patch from comment 5 was about 14mb smaller than the value from the build without the patch.

Comment 7

[Josh Matthews [:jdm]]

Additionally, in a testcase like https://bugzilla.mozilla.org/attachment.cgi?id=8906228 with significantly longer data URLs, this patch returns us to within 15mb of non-stylo heap-unclassified values; without the patch we're sitting at 4.5x more heap-unclassified.

Comment 8

[Simon Sapin (:SimonSapin)]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> So it's pretty ordinary URLs, just with *many* repeats.

We already use Arc (reference-counting) for URLs, so sharing memory for repeats would be nice. That would involve some sort of global or per-document cache indexed by input string to the URL parser. We could exclude data: URLs (which are likely longer), but even so hashing all these strings would cost time. Maybe it’s not worth optimizing too specifically for the pathological case of many repeats?

Comment 9

[Nicholas Nethercote [inactive]]

A few observations.

- SpecifiedUrl::serialization is currently an Arc<String> for no good reason; it's never shared. You can change it to a String just by removing the two Arc::new() calls.

- Browsing gmail and a few other sites (CNN, NYTimes, Twitter, etc.) I hit from_url_value_data() *zero* times. This is the function that jdm's patch modifies.

- In comparison, parse_from_string() is very common. jdm's patch doesn't modify that function.

- The non-gmail sites showed much repetition of the URLs. A bunch were repeated between 2--11 times, but nothing close to gmail's 1000+ counts.

jdm, I'm not sure how these intersect with your patch, but they might be useful to know.

Comment 10

[Nicholas Nethercote [inactive]]

Just to clarify (and possibly belabor the point): in the current code SpecifiedUrl has three allocations that are common for gmail:

- `serialization`, which is an Arc<String>, but could easily just be a String because it's not shared at all.

- `image_value`, which is an Option<RefPtr<ImageValue>>, which is created in in build_image_value(), via Gecko_ImageValue_Create(). ImageValue is 112 bytes on 64-bit.

- `image_value` contains a UTF-16 duplicate of `serialization`. (It also contains a hashtable, mRequests, though I haven't seen that show up notably in DMD output.)

Comment 11

[Josh Matthews [:jdm]]

The changes to from_url_value_data are not the focus point of this patch; the expected win comes from build_image_value, which is called any time we parse a URL value in Stylo.

Comment 12

[Josh Matthews [:jdm]]

Attachment: Share strings in URLDataValue with Rust

DMD does not show any unreported memory relating to parsing URL values through Stylo now.

Comment 13

[Josh Matthews [:jdm]]

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

Comment 14

[samlh]

Comment on attachment 8907803 [details] [diff] [review]
Share strings in URLDataValue with Rust

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

::: layout/style/nsCSSValue.cpp
@@ +2865,5 @@
> +  bool stringsEqual;
> +  if (mUsingRustString && aOther.mUsingRustString) {
> +    stringsEqual = GetRustString() == aOther.GetRustString();
> +  } else {
> +    stringsEqual = GetUTF16String() == GetUTF16String();

missing aOther.?

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 8907803 [details] [diff] [review]
Share strings in URLDataValue with Rust

Try runs is showing significant assertion issues.

Comment 16

[Josh Matthews [:jdm]]

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

Comment 17

[Josh Matthews [:jdm]]

Attachment: Share strings in URLDataValue with Rust

This try run looks much more reasonable (the failures are from another applied patch).

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8907940 [details] [diff] [review]
Share strings in URLDataValue with Rust

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

This generally looks good.  One main issue below.

::: dom/base/nsContentUtils.cpp
@@ +10688,5 @@
> +      return *current == '#';
> +    }
> +  }
> +
> +  return false;

It might be nice to factor out the body of this to a templated helper function that can work on both character types.

::: layout/style/nsCSSValue.cpp
@@ +44,5 @@
>    return false;
>  }
>  
> +static bool
> +MightHaveRef(const nsACString& aString)

Consider factoring this one out too.

@@ +2816,5 @@
> +: mURI(Move(aURI))
> +, mExtraData(Move(aExtraData))
> +, mStrings(aString)
> +, mURIResolved(true)
> +, mUsingRustString(true)

Nit: these ":"- and ","-starting lines should all be indented one level.

@@ +2825,5 @@
>  
>  css::URLValueData::URLValueData(const nsAString& aString,
>                                  already_AddRefed<URLExtraData> aExtraData)
> +  : mExtraData(Move(aExtraData))
> +  ,  mStrings(aString)

Nit: space

@@ +2889,5 @@
> +  }
> +  if (mUsingRustString && aOther.mUsingRustString) {
> +    return GetRustString() == aOther.GetRustString();
> +  }
> +  return GetUTF16String() == aOther.GetUTF16String();

DefinitelyEqualURIs can be called from style worker threads (under CalcStyleDifference), so I don't think it is safe to call GetUTF16String() here.  Should we have a function to return an nsString temporary instead?

@@ +2920,5 @@
> +}
> +
> +const nsString&
> +css::URLValueData::GetUTF16String() const
> +{

If we keep this function, MOZ_ASSERT(NS_IsMainThread()).

@@ +2941,5 @@
>      MOZ_ASSERT(!mURI);
>      nsCOMPtr<nsIURI> newURI;
> +    if (!mUsingRustString) {
> +      NS_NewURI(getter_AddRefs(newURI),
> +                NS_ConvertUTF16toUTF8(mStrings.mString),

Nice that we can avoid this extra conversion here. :-)

@@ +3143,5 @@
>  }
>  
> +css::ImageValue::ImageValue(ServoRawOffsetArc<RustString> aString,
> +                            already_AddRefed<URLExtraData> aExtraData)
> +: URLValueData(aString, Move(aExtraData))

Nit: indent one level.

@@ +3148,5 @@
> +{
> +}
> +
> +/*static*/
> +css::ImageValue*

Nit: "/* static */ css::ImageValue*" on one line.

@@ +3149,5 @@
> +}
> +
> +/*static*/
> +css::ImageValue*
> +css::ImageValue::CreateFromURLValue(URLValue* url,

Nit: "aURL".

::: layout/style/nsCSSValue.h
@@ +113,5 @@
> +  URLValueData(already_AddRefed<PtrHolder<nsIURI>> aURI,
> +               ServoRawOffsetArc<RustString> aString,
> +               already_AddRefed<URLExtraData> aExtraData);
> +  URLValueData(ServoRawOffsetArc<RustString> aString,
> +               already_AddRefed<URLExtraData> aExtraData);

Move this declaration up to just below the other one that takes a string but not an nsIURI.

@@ +175,3 @@
>    RefPtr<URLExtraData> mExtraData;
>  private:
> +  nsDependentCSubstring GetRustString() const;

Maybe add a comment here saying that this nsDependentCSubstring will point to mStrings.mRustString, so it wouldn't be safe to start returning it to external consumers.

@@ +177,5 @@
> +  nsDependentCSubstring GetRustString() const;
> +
> +  mutable union RustOrGeckoString {
> +    explicit RustOrGeckoString(const nsAString& aString) : mString(aString) {}
> +    explicit RustOrGeckoString(ServoRawOffsetArc<RustString> aString) : mRustString(aString) {}

Nit: please re-wrap to keep within 80 columns.

@@ +197,5 @@
>  private:
>    URLValueData(const URLValueData& aOther) = delete;
>    URLValueData& operator=(const URLValueData& aOther) = delete;
> +
> +  friend struct ImageValue;

Can we just make mStrings and mUsingRustString protected instead?

@@ +247,5 @@
> +  // must be called later for the object to be useful.
> +  ImageValue(ServoRawOffsetArc<RustString> aURIString,
> +             already_AddRefed<URLExtraData> aExtraData);
> +
> +  ImageValue(URLValue* url, nsIDocument* aDocument);

Is this declaration unused?

::: servo/components/style/gecko/url.rs
@@ +123,5 @@
>              unsafe {
> +                let arc_offset = Arc::into_raw_offset(self.serialization.clone());
> +                let ptr = Gecko_ImageValue_Create(
> +                    self.for_ffi(),
> +                    mem::transmute::<_, RawOffsetArc<RustString>>(arc_offset));

And are the type arguments needed here too?

::: servo/ports/geckolib/glue.rs
@@ +3832,5 @@
> +pub unsafe extern "C" fn Servo_CloneArcStringData(string: *const RawOffsetArc<RustString>)
> +                                                  -> RawOffsetArc<RustString> {
> +    let string = string as *const RawOffsetArc<String>;
> +    let cloned = (*string).clone();
> +    mem::transmute::<_, RawOffsetArc<RustString>>(cloned)

Are the type arguments necessary here?  (Can it work it out from the return type of the function?)

Comment 19

[Josh Matthews [:jdm]]

>::: servo/components/style/gecko/url.rs
>@@ +123,5 @@
>>              unsafe {
>> +                let arc_offset = Arc::into_raw_offset(self.serialization.clone());
>> +                let ptr = Gecko_ImageValue_Create(
>> +                    self.for_ffi(),
>> +                    mem::transmute::<_, RawOffsetArc<RustString>>(arc_offset));
>
>And are the type arguments needed here too?

They are not, but transmute is enough of a blunt hammer that I prefer to be precise about the target type in general.

Comment 20

[Josh Matthews [:jdm]]

>@@ +197,5 @@
>>  private:
>>    URLValueData(const URLValueData& aOther) = delete;
>>    URLValueData& operator=(const URLValueData& aOther) = delete;
>> +
>> +  friend struct ImageValue;
> 
>Can we just make mStrings and mUsingRustString protected instead?

Protected members can't be used from static functions for derived classes, unfortunately.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Share strings in URLDataValue with Rust

Comment 22

[Josh Matthews [:jdm]]

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

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8908203 [details] [diff] [review]
Share strings in URLDataValue with Rust

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

Looks good, thanks!

::: layout/style/nsCSSValue.cpp
@@ +3142,5 @@
>  }
>  
> +css::ImageValue::ImageValue(ServoRawOffsetArc<RustString> aString,
> +                            already_AddRefed<URLExtraData> aExtraData)
> +: URLValueData(aString, Move(aExtraData))

Nit: indent.

Comment 24

[Josh Matthews [:jdm]]

Attachment: Share strings in URLDataValue with Rust

Gecko changes that need to merge after https://github.com/servo/servo/pull/18516 lands.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

servo-vcs-sync is down, so this can't land until the commits have gone into autoland (see bug 1400205).

Comment 26

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/13ec4ded3b4e just synced over

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6159e19a7c0f
Share strings in URLDataValue with Rust. r=heycam

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6159e19a7c0f