XHR-related dark matter string

RESOLVED FIXED in mozilla21

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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.

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 563700
Whiteboard: [MemShrink]
(Assignee)

Comment 5

5 years ago
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.

Comment 7

5 years ago
nsGlobalWindow.mEventTargetObjects should keep pointers also to worker XHRs, at least until the 
window is destroyed.
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

4 years ago
> 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.
(Assignee)

Comment 10

4 years ago
> How are XHRs stored here?

Ehsan helped me with this.  nsXMLHttpRequest is a sub-class of nsXHREventTarget, which is a sub-class of nsDOMEventTargetHelper.
(Assignee)

Comment 11

4 years ago
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 :)
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
Created attachment 700815 [details] [diff] [review]
(part 1) - Add SizeOfIncludingThisEvenIfShared() for strings.

This patch adds SizeOf{In,Ex}cludingThisEvenIfShared to multiple string
classes.
Attachment #700815 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Assignee: nobody → n.nethercote
(Assignee)

Comment 17

4 years ago
Created attachment 700816 [details] [diff] [review]
(part 2) - Report memory used by event targets, especially XHRs.

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+
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Updated

4 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 21

4 years ago
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.
(Assignee)

Comment 23

4 years ago
Created attachment 703752 [details] [diff] [review]
(part 2) - Report memory used by event targets, especially XHRs.

New version that uses nsISizeOfEventTarget.
Attachment #703752 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 25

4 years ago
4 minutes 31 seconds for a review?  O_o
Well, I compared to the previous patch, which I'd already reviewed... ;)
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b924819d71a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/de2ab911692d
https://hg.mozilla.org/mozilla-central/rev/b924819d71a5
https://hg.mozilla.org/mozilla-central/rev/de2ab911692d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Updated

4 years ago
Duplicate of this bug: 740162
You need to log in before you can comment on or make changes to this bug.