If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add memory reporter for external strings

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: smaug, Assigned: bz)

Tracking

50 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

9 months ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1324430#c19
Created attachment 8822259 [details]
Testcase that should show this problem

This testcase confuses me.  It should allocate 2GB or so of external strings.  And stepping through in a debugger, I see us doing just that: mallocing a bunch of stringbuffers, handing each one over to the JS engine as an external string.  The confusing part is that when all is done about:memory does _not_ show 2GB of heap-unclassified, and if I ask the OS how much memory the process is using it's nowhere close to 2GB.  This is very confusing to me....

njn, any idea what's going on here?
Flags: needinfo?(n.nethercote)
Off the top of my head, I don't know what's happening. But the OS should be correct. I see 400 MiB of arrays (8 bytes per elements, which seems reasonable) but no strings. You see a bunch of strings being allocated -- do you see 5e7 of them?

I can take a closer look when I get back from PTO on Jan 9 if it's still a mystery then.
Flags: needinfo?(n.nethercote)
> You see a bunch of strings being allocated -- do you see 5e7 of them?

Oh, hmm.  Good point!  I bet what's going on here is that once we ion-compile we end up loop-hoisting the innerHTML get.  Let me check that hypothesis.
Created attachment 8822263 [details]
Testcase that shows this problem

I'm seeing a nice 85% or so heap-unclassified with this testcase.
I think that rather than overloading the finalizer function we pass in for external strings, or providing a separate function (bloating JS strings for little win), the best thing is to register some sort of callback for the JS engine memory reporting bits for when external strings are encountered during memory reporting.  Not sure whether we can attribute those external strings to the web page that caused them, though even just being able to say "hey, there's a pile of external strings here!" might be a useful thing to know...
Ok, so with a bit of plumbing, that testcase now gives me:

2,850.79 MB (100.0%) -- explicit
├──2,688.96 MB (94.32%) -- js-non-window
│  ├──2,680.50 MB (94.03%) -- zones
│  │  ├──2,672.75 MB (93.75%) -- zone(0x118ca7000)
│  │  │  ├──2,670.29 MB (93.67%) -- strings
│  │  │  │  ├──2,670.29 MB (93.67%) -- string(length=61, copies=9999999, "<div x="y">/n  <span>Please look at about:memory</span>/n</di")

which is better than nothing, but still doesn't pin the strings on a particular website, unfortunately.  I think that's a general problem with the way the JS engine reports string memory, though...
Created attachment 8822293 [details] [diff] [review]
part 1.  Add a way to set an external string memory runtime callback
Attachment #8822293 - Flags: review?(jwalden+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8822294 [details] [diff] [review]
part 2.  If there is an external string memory runtime callback, call it from memory reporting code
Attachment #8822294 - Flags: review?(jwalden+bmo)
Created attachment 8822295 [details] [diff] [review]
part 3.  Pass a useful external string memory reporter to SpiderMonkey from Gecko code
Attachment #8822295 - Flags: review?(nfroyd)
Comment on attachment 8822293 [details] [diff] [review]
part 1.  Add a way to set an external string memory runtime callback

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

::: js/src/jsapi.h
@@ +734,5 @@
> + * JSStringFinalizer passed to JS_NewExternalString for this string.
> + */
> +typedef size_t
> +(* JSExternalStringSizeofCallback)(JSString* str,
> +                                   mozilla::MallocSizeOf mallocSizeOf);

It's inconsistent with the surrounding code, but whatever, we prefer it now -- use a C++11-style typedef:

  using JSExternalStringSizeofCallback =
      size_t (*)(JSString* str, mozilla::MallocSizeOf mallocSizeOf);

::: js/src/vm/Runtime.h
@@ +843,5 @@
>  
>      /* Call this to get the name of a compartment. */
>      JSCompartmentNameCallback compartmentNameCallback;
>  
> +    /* Callback for doing memory reporting on external strings */

Period at end.
Attachment #8822293 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8822294 [details] [diff] [review]
part 2.  If there is an external string memory runtime callback, call it from memory reporting code

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

::: js/src/vm/String.cpp
@@ +56,5 @@
> +    // JSExternalString: Ask the embedding to tell us what's going on.  If it
> +    // doesn't want to say, don't count, the chars could be stored anywhere.
> +    if (isExternal()) {
> +        JSRuntime* rt = runtimeFromMainThread();
> +        if (rt->externalStringSizeofCallback)

I'd do

  if (auto* cb = runtimeFromMainThread()->externalStringSizeofCallback)
    return cb(this, mallocSizeOf);

which also happens to make clear that the callback is a function pointer and not a member function that might get passed the runtime.
Attachment #8822294 - Flags: review?(jwalden+bmo) → review+
Addressed those comments.
Comment on attachment 8822295 [details] [diff] [review]
part 3.  Pass a useful external string memory reporter to SpiderMonkey from Gecko code

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

Thank you!
Attachment #8822295 - Flags: review?(nfroyd) → review+

Comment 14

9 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b94a2497dc
part 1.  Add a way to set an external string memory runtime callback.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/52d6c09777ee
part 2.  If there is an external string memory runtime callback, call it from memory reporting code.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a2ffd6f792
part 3.  Pass a useful external string memory reporter to SpiderMonkey from Gecko code.  r=froydnj

Comment 15

9 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/defd98672958
followup.  Suppress the false-positive GC analysis failure due to us doing an indirect call that the static analysis doesn't understand.  r=sfink

Comment 16

9 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2b070d0054
another followup: fix build bustage on CLOSED TREE.

Comment 17

9 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e5976fb87a
yet another followup.  Silence the opinionated spidermonkey include style checker on CLOSED TREE

Comment 18

9 months ago
Backout by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc7373573e9
Back out bits that should not have landed for bug 1326096 on CLOSED TREE.

Comment 19

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72e5976fb87a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.