1324430 - twitter causes high heap-unclassified with many new tweets

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

After coming back to my desktop after the weekend I noticed my twitter tab had 14,000+ tweets pending.  I was wondering how bad the memory situation was with that, so I took a measurement.  Surprisingly, the largest entry in the child process was for heap-unclassified at 2.5GB.

Since this was a production build I did not have DMD enabled.  I tried instead to incrementally close tabs to see what made the number go down.

Closing my window with gmail, irccloud, and telegram caused heap-unclassified to drop from 2.5GB to 1.5GB.  At that point only twitter.com was open.

Refreshing the twitter.com page to drop all those pending tweets caused heap-unclassified to drop to a normal level.  Something like 35MB.

I don't know exactly whats going on, but it feels like we have some new per-element overhead that we are not accounting for.  Pending tweets are held as non-visible DOM elements.  Or perhaps we are tracking elements in a list somewhere that is doing a doubling growth algorithm which is producing the large memory usage.

I'll see if I can get a workable DMD build locally to try to reproduce.

This was all in FF52 dev-edition.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'm at 1100 pending tweets at the moment and these are the largest DMD allocations:

Unreported {
  180 blocks in heap block record 1 of 5,773
  46,680,064 bytes (46,402,312 requested / 277,752 slop)
  Individual block sizes: 1,052,672 x 42; 528,384 x 4; 36,864 x 6; 2,048 x 2; 1,024 x 126
  13.96% of the heap (13.96% cumulative)
  27.38% of unreported (27.38% cumulative)
  Allocated at {
    #01: nsStringBuffer::Alloc (c:\devel\mozilla-central\xpcom\string\nssubstring.cpp:218)
    #02: nsAString_internal::MutatePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:149)
    #03: nsAString_internal::ReplacePrepInternal (c:\devel\mozilla-central\xpcom\string\nstsubstrin>
    #04: nsAString_internal::ReplacePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:20>
    #05: nsAString_internal::Assign (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:364)
    #06: mozilla::dom::XMLHttpRequestStringSnapshot::GetAsString (c:\devel\mozilla-central\dom\xhr\>
    #07: mozilla::dom::XMLHttpRequestMainThread::GetResponseText (c:\devel\mozilla-central\dom\xhr\>
    #08: mozilla::dom::XMLHttpRequestBinding::get_responseText (c:\devel\mozilla-central\obj-x86_64>
    #09: mozilla::dom::GenericBindingGetter (c:\devel\mozilla-central\dom\bindings\bindingutils.cpp>
    #10: js::jit::DoCallNativeGetter (c:\devel\mozilla-central\js\src\jit\sharedic.cpp:2587)
    #11: ??? (???:???)
  }
}

Unreported {
  1,084 blocks in heap block record 2 of 5,773
  44,515,328 bytes (40,086,104 requested / 4,429,224 slop)
  Individual block sizes: 135,168; 69,632 x 136; 36,864 x 947
  13.31% of the heap (27.26% cumulative)
  26.11% of unreported (53.48% cumulative)
  Allocated at {
    #01: nsStringBuffer::Alloc (c:\devel\mozilla-central\xpcom\string\nssubstring.cpp:218)
    #02: nsAString_internal::MutatePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:149)
    #03: nsAString_internal::SetCapacity (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:68>
    #04: `anonymous namespace'::StringBuilder::ToString (c:\devel\mozilla-central\dom\base\nsconten>
    #05: nsContentUtils::SerializeNodeToMarkup (c:\devel\mozilla-central\dom\base\nscontentutils.cp>
    #06: mozilla::dom::FragmentOrElement::GetMarkup (c:\devel\mozilla-central\dom\base\fragmentorel>
    #07: mozilla::dom::Element::GetInnerHTML (c:\devel\mozilla-central\dom\base\element.cpp:3511)
    #08: mozilla::dom::ElementBinding::get_innerHTML (c:\devel\mozilla-central\obj-x86_64-pc-mingw3>
    #09: mozilla::dom::GenericBindingGetter (c:\devel\mozilla-central\dom\bindings\bindingutils.cpp>
    #10: js::jit::DoCallNativeGetter (c:\devel\mozilla-central\js\src\jit\sharedic.cpp:2587)
    #11: ??? (???:???)
  }

Seems like strings passed off to js?  Jan, do we account for this memory from the js memory reporter at all?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Yea, after letting this run overnight I had 6000+ pending tweets.  The top stacks are more or less the same, just with more memory:

Unreported {
  1,338 blocks in heap block record 1 of 5,810
  269,548,544 bytes (267,483,716 requested / 2,064,828 slop)
  Individual block sizes: 1,052,672 x 196; 528,384 x 88; 266,240 x 44; 135,168 x 17; 69,632 x 7; 36,
864 x 33; 12,288 x 2; 8,192; 2,048 x 5; 1,024 x 945
  28.90% of the heap (28.90% cumulative)
  42.32% of unreported (42.32% cumulative)
  Allocated at {
    #01: nsStringBuffer::Alloc (c:\devel\mozilla-central\xpcom\string\nssubstring.cpp:218)
    #02: nsAString_internal::MutatePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:149)
    #03: nsAString_internal::ReplacePrepInternal (c:\devel\mozilla-central\xpcom\string\nstsubstring
.cpp:211)
    #04: nsAString_internal::ReplacePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:203
)
    #05: nsAString_internal::Assign (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:364)
mlhttprequeststring.cpp:194)
    #07: mozilla::dom::XMLHttpRequestMainThread::GetResponseText (c:\devel\mozilla-central\dom\xhr\x
mlhttprequestmainthread.cpp:588)
    #08: mozilla::dom::XMLHttpRequestBinding::get_responseText (c:\devel\mozilla-central\obj-x86_64-
pc-mingw32\dom\bindings\xmlhttprequestbinding.cpp:1183)
    #09: mozilla::dom::GenericBindingGetter (c:\devel\mozilla-central\dom\bindings\bindingutils.cpp:
2854)
    #10: js::jit::DoCallNativeGetter (c:\devel\mozilla-central\js\src\jit\sharedic.cpp:2587)
    #11: ??? (???:???)
  }
}

Unreported {
  5,810 blocks in heap block record 2 of 5,810
  245,030,912 bytes (221,291,252 requested / 23,739,660 slop)
  Individual block sizes: 135,168 x 3; 69,632 x 934; 36,864 x 4,870; 20,480 x 3
  26.28% of the heap (55.18% cumulative)
  38.47% of unreported (80.79% cumulative)
  Allocated at {
  Allocated at {
    #01: nsStringBuffer::Alloc (c:\devel\mozilla-central\xpcom\string\nssubstring.cpp:218)
    #02: nsAString_internal::MutatePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:149)

    #03: nsAString_internal::SetCapacity (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:687
)
    #04: `anonymous namespace'::StringBuilder::ToString (c:\devel\mozilla-central\dom\base\nscontent
utils.cpp:8847)
    #05: nsContentUtils::SerializeNodeToMarkup (c:\devel\mozilla-central\dom\base\nscontentutils.cpp
:9307)
    #06: mozilla::dom::FragmentOrElement::GetMarkup (c:\devel\mozilla-central\dom\base\fragmentorele
ment.cpp:2151)
    #07: mozilla::dom::Element::GetInnerHTML (c:\devel\mozilla-central\dom\base\element.cpp:3511)
    #08: mozilla::dom::ElementBinding::get_innerHTML (c:\devel\mozilla-central\obj-x86_64-pc-mingw32
\dom\bindings\elementbinding.cpp:2788)
    #09: mozilla::dom::GenericBindingGetter (c:\devel\mozilla-central\dom\bindings\bindingutils.cpp:
2854)
    #10: js::jit::DoCallNativeGetter (c:\devel\mozilla-central\js\src\jit\sharedic.cpp:2587)
    #11: ??? (???:???)
  }
}

Unreported {
  343 blocks in heap block record 3 of 5,810
  59,498,768 bytes (58,750,566 requested / 748,202 slop)
  Individual block sizes: 2,101,248 x 3; 1,052,672 x 33; 528,384 x 13; 266,240 x 18; 135,168 x 27; 6
9,632 x 21; 36,864 x 37; 20,480 x 5; 8,192; 2,048 x 20; 1,024 x 164; 272
  6.38% of the heap (61.56% cumulative)
  9.34% of unreported (90.13% cumulative)
  Allocated at {
    #01: nsStringBuffer::Alloc (c:\devel\mozilla-central\xpcom\string\nssubstring.cpp:218)
    #02: nsAString_internal::MutatePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:149)
    #03: nsAString_internal::ReplacePrepInternal (c:\devel\mozilla-central\xpcom\string\nstsubstring
.cpp:211)
    #04: nsAString_internal::ReplacePrep (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:203
)
    #05: nsAString_internal::Assign (c:\devel\mozilla-central\xpcom\string\nstsubstring.cpp:364)
    #06: mozilla::dom::XMLHttpRequestStringSnapshot::GetAsString (c:\devel\mozilla-central\dom\xhr\x
mlhttprequeststring.cpp:194)
    #07: mozilla::dom::XMLHttpRequestMainThread::GetResponseText (c:\devel\mozilla-central\dom\xhr\x
mlhttprequestmainthread.cpp:588)
    #08: mozilla::dom::XMLHttpRequestBinding::get_responseText (c:\devel\mozilla-central\obj-x86_64-
pc-mingw32\dom\bindings\xmlhttprequestbinding.cpp:1183)
    #09: mozilla::dom::GenericBindingGetter (c:\devel\mozilla-central\dom\bindings\bindingutils.cpp:
2854)
    #10: js::InternalCallOrConstruct (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:457)
    #11: js::CallGetter (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:635)
    #12: CallGetter (c:\devel\mozilla-central\js\src\vm\nativeobject.cpp:1809)
    #13: js::NativeGetProperty (c:\devel\mozilla-central\js\src\vm\nativeobject.cpp:2118)
    #14: js::GetProperty (c:\devel\mozilla-central\js\src\jsobj.h:844)
    #15: js::GetProperty (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:4273)
    #16: GetPropertyOperation (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:192)
    #17: Interpret (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:2636)
    #18: js::RunScript (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:403)
    #19: js::InternalCallOrConstruct (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:478)
    #20: js::Call (c:\devel\mozilla-central\js\src\vm\interpreter.cpp:521)
    #21: JS::Call (c:\devel\mozilla-central\js\src\jsapi.cpp:2830)
    #22: mozilla::dom::EventHandlerNonNull::Call (c:\devel\mozilla-central\obj-x86_64-pc-mingw32\dom
\bindings\eventhandlerbinding.cpp:259)
    #23: mozilla::dom::EventHandlerNonNull::Call<nsISupports * __ptr64> (c:\devel\mozilla-central\ob
j-x86_64-pc-mingw32\dist\include\mozilla\dom\eventhandlerbinding.h:361)
    #24: mozilla::JSEventHandler::HandleEvent (c:\devel\mozilla-central\dom\events\jseventhandler.cp
p:215)
  }
}

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Boris, do you have any thoughts here?  How is memory for strings returned from DOM bindings to js supposed to be accounted for?

Comment 4

[Jan de Mooij [:jandem]]

The JS engine should report the memory for all strings owned by it. External string chars, however, are not owned by the engine itself, so we return 0 for those (see JSString::sizeOfExcludingThis).

Maybe we don't have a memory reporter for that on the DOM side?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

It seems like once we call this:

  https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcpublic.h#246

We don't account for the memory anywhere on the DOM side either.

We could easily track a static global value for the entire process, but it would be better to account it against the window and/or sub-system.  Its hard to do that, though, since JS_NewExternalString() takes a callback function with no context parameter.  I don't see how we can pass through an object on which to do the accounting.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I guess the added difficulty here is this becomes untracked memory only when there are no nsString objects in gecko referencing the same buffer.  It only happens when the js engine ref is the last ref to the buffer.

Comment 7

[Nathan Froyd [:froydnj]]

Well, if JS was iterating through and noticed an external string, it could at least call some sort of callback into Gecko, and that would give us an opportunity to report the string if it was unshared...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Isn't that about the new string sharing stuff baku added to XHR.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Isn't that about the new string sharing stuff baku added to XHR.

Partially.  One of the stacks is from reading .innerHTML, though.  I think this is more a general binding layer issue.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> How is memory for strings returned from DOM bindings to js supposed to be accounted for?

It depends.  If we end up sharing an actual stringbuffer the DOM is also storing, then it won't be accounted for in any way right now.  In particular, I see no callers of SizeOfExcludingThisEvenIfShared/SizeOfExcludingThisEvenIfShared in our codebase.  We _used_ to have it called from XHR code (see bug 826521), but it looks to me like the changes for bug 1249739 changed this.  In particular, XMLHttpRequestStringBuffer::SizeOfThis calls SizeOfExcludingThisIfUnshared.  Unfortunately, the big "yes, we're reporting this even if shared" comment in XMLHttpRequestMainThread::SizeOfEventTargetIncludingThis was left in place, which makes the code really confusing.

On the other hand, our allocation strategy here also looks like it's totally changed.  XMLHttpRequestStringBuffer::GetAsString looks to me like it always copies, never shares.  Is that correct?  Is there a reason we aren't sharing here?  I'd think we could just share the buffer here (and restore the old "report even if shared") behavior.

> I guess the added difficulty here is this becomes untracked memory only when there are
> no nsString objects in gecko referencing the same buffer.

Actually, it's untracked in the worst way precisely when there's an nsString in Gecko _and_ a JSString both referencing the same thing, because we don't track shared buffers with refcount > 1 per above.  :(

But yes, for the innerHTML case we don't report the thing even though it's unshared.  Comment 7 is exactly the right way to handle that: a runtime callback that gets handed the external string.  Then we can check XPCStringConvert::IsDOMString and if so extract the stringbuffer (see how that's done in XPCConvert::JSData2Native) and report it if unshared.

Comment 11

[Boris Zbarsky [:bzbarsky]]

ccing njn too, in case he has any other ideas.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I'd think we could just share the buffer here

Specifically, I think we should do the following for the sharing situation in XHR:

1)  Add a nsTSubstring_CharT::Assign(const self_type& aStr, size_type aLength, const fallible_t& aFallible) function that does something sane to ensure aLength <= aStr.Length() (e.g. just clamps it to that?) and otherwise behaves identically to our existing nsTSubstring_CharT::Assign(const self_type& aStr, const fallible_t& aFallible), but sets its length to (clamped) aLength, not aStr.Length().

2)  Use this function in XMLHttpRequestStringBuffer::GetAsString, so we get sharing.

Comment 13

[Boris Zbarsky [:bzbarsky]]

In particular, we could just have nsTSubstring_CharT::Assign(const self_type& aStr, const fallible_t& aFallible) do:

  Assign(aStr, aStr.Length(), aFallible)

and move all the real work into the new signature.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #10)
> On the other hand, our allocation strategy here also looks like it's totally
> changed.  XMLHttpRequestStringBuffer::GetAsString looks to me like it always
> copies, never shares.  Is that correct? 
Clearly not. We should be able to share easily when 
XMLHttpRequestStringBuffer::mData's length is the same as the length passed to GetAsString()

Or optimize more using comment 12 and comment 13.

Random note, we also allocate possibly way too much in string code, because we may spill out to the next jemalloc bucket since nsStringBuffer's allocation methods add sizeof(nsStringBuffer) to the requested size. Bug 1324808

Comment 15

[Andrea Marchesini [:baku]]

Attachment: xhr.patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8821224 [details] [diff] [review]
xhr.patch

nsTSubstring.h could use some comments about what aLength means and how it interacts with aStr.Length().  (Not just marking r+, because I feel like this was enough my idea that it could use a second set of eyes on it in terms of the string API, but apart from the missing comments, this looks good.)

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8821224 [details] [diff] [review]
xhr.patch

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

Thanks for implementing this.

I shudder to think of possible problems this enables with string buffer sharing and mutating shared strings, but I guess it is no different than scenarios we already have.

::: xpcom/string/nsTSubstring.h
@@ +380,5 @@
>  
> +  void NS_FASTCALL Assign(const self_type& aStr, size_type aLength);
> +  MOZ_MUST_USE bool NS_FASTCALL Assign(const self_type& aStr,
> +                                       size_type aLength,
> +                                       const fallible_t&);

Please provide a comment here, something like:

// Assign at most aLength characters of aStr to this, fallibly.
// If aLength is more than the number of characters in aStr.Length(),
// only assign aStr.Length() characters.
//
// Using this method is functionally equivalent to:
//
//   str.Assign(other.BeginReading, other.Length(), fallible);
//
// but may enable string buffer sharing.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03429256e51e
Implement nsTSubstring_CharT::Assign with length for a better string buffer sharing, r=froydnj

Comment 19

[Boris Zbarsky [:bzbarsky]]

We should get a followup file for a memory reporter for external strings, right?

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/03429256e51e

Comment 21

[Boris Zbarsky [:bzbarsky]]

smaug filed bug 1326096 for the memory reporter followup.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Andrea, what branch did the XHR string stuff land on?  Do we need to uplift this?

Comment 23

[Andrea Marchesini [:baku]]

It lands the 2016-12-24, it should be 53.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8821224 [details] [diff] [review]
xhr.patch

Approval Request Comment
[Feature/Bug causing the regression]:bug 1249739.
[User impact if declined]: some string data is not reported correctly.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: I guess so. I did it locally.
[Needs manual test from QE? If yes, steps to reproduce]: follow comment 1.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This patch touches how content is assigned to strings. This is a critical part of gecko code. The fact that nightly is stable, means that this patch is not risky.
[String changes made/needed]: none

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8821224 [details] [diff] [review]
xhr.patch

fix memory reporting for some string data, aurora52+

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4b165800dd7