1326096 - Add memory reporter for external strings

Status: RESOLVED FIXED

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

Description

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

https://bugzilla.mozilla.org/show_bug.cgi?id=1324430#c19

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: 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?

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows this problem

I'm seeing a nice 85% or so heap-unclassified with this testcase.

Comment 5

[Nathan Froyd [:froydnj]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to set an external string memory runtime callback

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. If there is an external string memory runtime callback, call it from memory reporting code

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Pass a useful external string memory reporter to SpiderMonkey from Gecko code

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Addressed those comments.

Comment 13

[Nathan Froyd [:froydnj]]

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!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2b070d0054
another followup: fix build bustage on CLOSED TREE.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e5976fb87a
yet another followup.  Silence the opinionated spidermonkey include style checker on CLOSED TREE

Comment 18

[Pulsebot]

Backout by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72e5976fb87a