Closed Bug 826521 Opened 7 years ago Closed 7 years ago

XHR-related dark matter string

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 1 obsolete file)

I did an 8 hour DMD run of my normal daily browsing.  The top unreported stack
trace record was this:

> Unreported: 1 block in stack trace record 1 of 4,082
>  2,101,248 bytes (2,097,226 requested / 4,022 slop)
>  0.70% of the heap (0.70% cumulative);  3.92% of unreported (3.92% cumulative)
>  Allocated at
>    nsStringBuffer::Realloc(nsStringBuffer*, unsigned long) (/home/njn/moz/mi0/xpcom/string/src/nsSubstring.cpp:207) 0x7f0e2272f12d
>    nsAString_internal::MutatePrep(unsigned int, unsigned short**, unsigned int*) (/home/njn/moz/mi0/xpcom/string/src/nsTSubstring.cpp:103) 0x7f0e2272f366
>    nsAString_internal::SetCapacity(unsigned int, mozilla::fallible_t const&) (/home/njn/moz/mi0/xpcom/string/src/nsTSubstring.cpp:553) 0x7f0e2272fd36
>    nsXMLHttpRequest::AppendToResponseText(char const*, unsigned int) (/home/njn/moz/mi0/content/base/src/nsXMLHttpRequest.cpp:776) 0x7f0e21c3f543
>    nsXMLHttpRequest::StreamReaderFunc(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (/home/njn/moz/mi0/content/base/src/nsXMLHttpRequest.cpp:1901) 0x7f0e21c420c8
>    nsPipeInputStream::ReadSegments(tag_nsresult (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (/home/njn/moz/mi0/xpcom/io/nsPipe3.cpp:775) 0x7f0e226fd456
>    nsXMLHttpRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (/home/njn/moz/mi0/content/base/src/nsXMLHttpRequest.cpp:1980) 0x7f0e21c4242a
>    nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (/home/njn/moz/mi0/netwerk/base/src/nsBaseChannel.cpp:764) 0x7f0e217a4dc5
>    nsInputStreamPump::OnStateTransfer() (/home/njn/moz/mi0/netwerk/base/src/nsInputStreamPump.cpp:482) 0x7f0e217ae9f4
>    nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/njn/moz/mi0/netwerk/base/src/nsInputStreamPump.cpp:371) 0x7f0e217ae738
>    nsInputStreamReadyEvent::Run() (/home/njn/moz/mi0/xpcom/io/nsStreamUtils.cpp:83) 0x7f0e226fec70
>    nsThread::ProcessNextEvent(bool, bool*) (/home/njn/moz/mi0/xpcom/threads/nsThread.cpp:627) 0x7f0e227123b4
>    NS_ProcessNextEvent_P(nsIThread*, bool) (/home/njn/moz/mi0/dmdo64/xpcom/build/nsThreadUtils.cpp:237) 0x7f0e226d8ac9
>    mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/njn/moz/mi0/ipc/glue/MessagePump.cpp:82) 0x7f0e2252833c
>    MessageLoop::Run() (/home/njn/moz/mi0/ipc/chromium/src/base/message_loop.cc:183) 0x7f0e22741d71
>    nsBaseAppShell::Run() (/home/njn/moz/mi0/widget/xpwidgets/nsBaseAppShell.cpp:165) 0x7f0e2244e2eb
>    nsAppStartup::Run() (/home/njn/moz/mi0/toolkit/components/startup/nsAppStartup.cpp:288) 0x7f0e222daf38
>    XREMain::XRE_mainRun() (/home/njn/moz/mi0/toolkit/xre/nsAppRunner.cpp:3823) 0x7f0e2177c3cb
>    XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/njn/moz/mi0/toolkit/xre/nsAppRunner.cpp:3890) 0x7f0e2177ca40
>    XRE_main (/home/njn/moz/mi0/toolkit/xre/nsAppRunner.cpp:4088) 0x7f0e2177ce81
>    do_main(int, char**) (/home/njn/moz/mi0/browser/app/nsBrowserApp.cpp:174) 0x402f95
>    main (/home/njn/moz/mi0/browser/app/nsBrowserApp.cpp:279) 0x402b56
>    __libc_start_main (/build/buildd/eglibc-2.15/csu/libc-start.c:258) 0x7f0e247b176d
>    _start (??:?) 0x402929
This allocation ends up in nsXMLHttpRequest::mResponseText.  nsXMLHttpRequest doesn't have any SizeOf methods, interestingly.  I don't know where the nsXMLHttpRequest will be stored, though.
nsXMLHttpRequest will be owned by the JSObject wrapper, in general.
We could add SizeOf to all the nsDOMEventTargetHelpers and enumerate nsGlobalWindow.mEventTargetObjects
That should catch most of XHRs.
The orphan finder could be modified to look for XHR.  QI to XHR in addition to node, or whatever, and call a sizeof method if it works.
Blocks: DarkMatter
Whiteboard: [MemShrink]
Notes from talking to bz on IRC:

XHRs are WebIDL objects, which means the nsISupports is not stored in the private slot but in slot 0.  Such objects have JSCLASS_IS_DOMJSCLASS set instead of JSCLASS_HAS_PRIVATE and JSCLASS_PRIVATE_IS_NSISUPPORTS.

I can extract an XHR from a JSObject like this:

  nsXMLHttpRequest* xhr;
  nsresult rv = mozilla::dom::UnwrapObject<nsXMLHttpRequest>(cx, obj, xhr);
  if (NS_SUCCEEDED(rv)) {
    ...
  }

|cx| can be NULL because we know this is a DOMJSCLASS, and therefore not a cross-compartment wrapper, and therefore no security check is needed.
NB: There are also nsXMLHttpRequests only used from C++, such as the underlying object for worker thread XHR.
nsGlobalWindow.mEventTargetObjects should keep pointers also to worker XHRs, at least until the 
window is destroyed.
To summarize:

1. The nsGlobalWindow.mEventTargetObjects approach will get worker XHRs, but will miss orphaned XHRs.

2. The JSObject/UnwrapObject will miss worker XHRs, but will get orphaned XHRs.

Hmm.  I might just do (1), and then (2) later if DMD indicates orphaned XHRs are significant.
> We could add SizeOf to all the nsDOMEventTargetHelpers and enumerate
> nsGlobalWindow.mEventTargetObjects
> That should catch most of XHRs.

So... nsGlobalWindow::mEventTargetObjects looks like this:

  nsTHashtable<nsPtrHashKey<nsDOMEventTargetHelper> > mEventTargetObjects;

How are XHRs stored here?  I couldn't work that out even after looking at nsDOMEventTargetHelper and the classes it inherits from, such as mozilla::dom::EventTarget, and nsIDomEventTarget.
> How are XHRs stored here?

Ehsan helped me with this.  nsXMLHttpRequest is a sub-class of nsXHREventTarget, which is a sub-class of nsDOMEventTargetHelper.
I have a draft patch but I've hit a problem.  nsXHR::mResponseText is the one that takes up all the space, but it's a shared string, and currently we don't have a way to measure shared strings.

Is anyone (bz?) able to tell me what other structure might be sharing that string?  Depending on the answer, it might be worth adding a SizeOfExcludingThisEvenIfShared() function that is suitable for use if you know that other references won't measure the string.

Alternatively, we could divide the size by the refcount and teach DMD about fractional blocks (whatever that means).
FWIW we saw yesterday the calendar app having many mb of heap-unclassified due to this.
By default it is not shared with anything, but if someone does something like
var text = XHR.responseText; it gets shared with JS. I don't know whether we report external
strings as JS mem usage.

I think you could check the refcnt of the string buffer. If it is 1 (== !ReadOnly), the string 
certainly isn't shared with anything. So, something like 
nsStringBuffer* b = nsStringBuffer::FromString(mResponseText);
if (b && !b->ReadOnly()) {
  // report memory usage...
}

But yes, in general perhaps memory reporter should understand shared string buffers and
for each case the string buffer is used size/refcnt.
Oh, we have SizeOfIncludingThisIfUnshared in nsStringBuffer :)
(In reply to Olli Pettay [:smaug] from comment #13)
> By default it is not shared with anything, but if someone does something like
> var text = XHR.responseText; it gets shared with JS. I don't know whether we
> report external
> strings as JS mem usage.

External string chars aren't counted by the JS reporter:  http://mxr.mozilla.org/mozilla-central/source/js/src/vm/String.cpp#61.

Is there any way to tell that the XHR is sharing the StringBuffer with a JS external string?

> But yes, in general perhaps memory reporter should understand shared string
> buffers and for each case the string buffer is used size/refcnt.

Will that miss some of the memory when the sharing is with a JS external string?  E.g. from the XHR reporter we'll report N/2 bytes, and from the JS reporter we'll report 0 bytes.  I guess if we knew that the JS external string's chars were in a StringBuffer we could check the refcount, but is it possible to know that from the JS engine side?

The other tricky part of the N/refcnt approach is DMD.  It would need to support the notion of a partially reported block.  It's probably doable, but it certainly complicates things.
This patch adds SizeOf{In,Ex}cludingThisEvenIfShared to multiple string
classes.
Attachment #700815 - Flags: review?(bzbarsky)
Assignee: nobody → n.nethercote
This patch adds measurement for the event targets object table, and XHRs, under
"dom/event-targets".  In Gmail I've seen this account for 6 MiB of memory.

The patch also does some trivial clean-ups:

- Moves the "dom/other" reports last, to match the struct field order.

- Simplifies the descriptions for the window sum reports, to avoid some
  repetition.

- Removes some trailing whitespace.

- Fixes some mode lines.
Attachment #700816 - Flags: review?(bzbarsky)
Comment on attachment 700815 [details] [diff] [review]
(part 1) - Add SizeOfIncludingThisEvenIfShared() for strings.

r=me
Attachment #700815 - Flags: review?(bzbarsky) → review+
Comment on attachment 700816 [details] [diff] [review]
(part 2) - Report memory used by event targets, especially XHRs.

Worth documenting that these objects do NOT include DOM nodes....  Are we sure none of the other ones are already being reported, by the way?  r=me if we've checked the subclasses of nsDOMEventTargetHelper to make sure they're not already reported.
Attachment #700816 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #19)
> Comment on attachment 700816 [details] [diff] [review]
> (part 2) - Report memory used by event targets, especially XHRs.
> 
> Worth documenting that these objects do NOT include DOM nodes....  Are we
> sure none of the other ones are already being reported, by the way?  r=me if
> we've checked the subclasses of nsDOMEventTargetHelper to make sure they're
> not already reported.

They'd have to implement nsISizeOf, and the only classes other than nsXHR to do that currently are:  nsSimpleURI, nsStandardURL and nsNullPrincipalURI.

And I checked the subclasses of nsDOMEventTargetHelper, and didn't recognize any other them.

But neither of those things is a terribly strong protection against double-counting in the long term.  Hmm.
Whiteboard: [MemShrink] → [MemShrink:P2]
So this patch relies on nsXHR implementing nsISizeOf.  The danger is if another subclass of nsDOMEventTargetHelper implements nsISizeOf, we will start counting it, which may not be what we want.

What about creating an interface nsISizeOfXHR that has the same methods as nsISizeOf?  nsXHR would implement it, nothing else would, and we'd be safe.  To generalize it slightly could instead name it nsISizeOfGlobalWindowEventTargetObjects, and potentially reuse it for other subclasses of nsDOMEventTargetHelper in the future.

Is that too grotty for words?
nsISizeOfEventTarget would be fine, I think.
New version that uses nsISizeOfEventTarget.
Attachment #703752 - Flags: review?(bzbarsky)
Attachment #700816 - Attachment is obsolete: true
Comment on attachment 703752 [details] [diff] [review]
(part 2) - Report memory used by event targets, especially XHRs.

r=me
Attachment #703752 - Flags: review?(bzbarsky) → review+
4 minutes 31 seconds for a review?  O_o
Well, I compared to the previous patch, which I'd already reviewed... ;)
https://hg.mozilla.org/mozilla-central/rev/b924819d71a5
https://hg.mozilla.org/mozilla-central/rev/de2ab911692d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Duplicate of this bug: 740162
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.