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.

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

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.

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).

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.

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

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.

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.

(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?

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.

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.)

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.

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

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 on attachment 8907803 [details] [diff] [review] Share strings in URLDataValue with Rust Try runs is showing significant assertion issues.

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

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?)

>::: 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.

>@@ +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 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.

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

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

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

Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6159e19a7c0f Share strings in URLDataValue with Rust. r=heycam