1487217 - We don't seem to have memory reporting for WeakMap hashtables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

From a DMD report:

Unreported {
  524 blocks in heap block record 38 of 6,988
  536,576 bytes (402,432 requested / 134,144 slop)
  Individual block sizes: 1,024 x 524
  0.11% of the heap (26.83% cumulative)
  0.30% of unreported (72.88% cumulative)
  Allocated at {
    #01: replace_malloc(unsigned long) (DMD.cpp:1267, in libmozglue.dylib)
    #02: mozilla::detail::HashTableEntry<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> > >* js::MallocProvider<JS::Zone>::pod_malloc<mozilla::detail::HashTableEntry<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> > > >(unsigned long) (Utility.h:387, in XUL)
    #03: mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >, mozilla::HashMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value>, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(unsigned int, mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >, mozilla::HashMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value>, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (Zone.h:788, in XUL)
    #04: bool mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >, mozilla::HashMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value>, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::add<JS::Handle<JSObject*>&, JS::Handle<JS::Value>&>(mozilla::detail::HashTable<mozilla::HashMapEntry<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >, mozilla::HashMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value>, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::AddPtr&, JS::Handle<JSObject*>&&&, JS::Handle<JS::Value>&&&) (HashTable.h:2171, in XUL)
    #05: bool mozilla::HashMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value>, js::MovableCellHasher<js::HeapPtr<JSObject*> >, js::ZoneAllocPolicy>::put<JS::Handle<JSObject*>&, JS::Handle<JS::Value>&>(JS::Handle<JSObject*>&&&, JS::Handle<JS::Value>&&&) (HashTable.h:278, in XUL)
    #06: WeakMap_set(JSContext*, unsigned int, JS::Value*) (WeakMapObject-inl.h:60, in XUL)
    #07: ??? (???:???)
    #08: ??? (???:???)
  }
}

Comment 1

[Jon Coppeard (:jonco)]

Indeed, we don't seem to have this.

The simplest fix would be to add cases to JSObject::addSizeOfExcludingThis for WeakMapObject and WeakSetObject.  Possibly a better fix would be to add a class hook for memory reporting and ditch this if statement.

Comment 2

[jlogandavison]

Hi there, I'm interested in picking up this bug. Is it available?

Comment 3

[Jon Coppeard (:jonco)]

By all means :)

Comment 4

[jlogandavison]

Hi there, I've been having a look a what's required for this.

The basic approach is to add a case to JSObject::addSizeOfExcludingThis for WeakCollectionObject. Then, a new method needs to be implemented for WeakCollectionObject (called possibly sizeOfData?) that returns `mallocSizeOf(getPrivate())` or `mallocSizeOf(getMap())` maybe. Am I on the right track?


Also, would you be able to elaborate on the other solution you mentioned involving a class hook? Guessing this is some way of calling the correct code directly without marching through a if-else-if chain, right? The way I could think of to achieve that would be to make addSizeOfExcludingThis a virtual method in the JSObject base class. Then have the special cases override/extend on that. Not sure of the performance implications of using virtual methods though. Is that what you meant by class hook?

Comment 5

[Nicholas Nethercote [inactive]]

I'd just go with the simpler fix for now.  What you suggested for that sounds reasonable.

https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting might be helpful, too.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to jlogandavison from comment #4)

> The basic approach is to add a case to JSObject::addSizeOfExcludingThis for
> WeakCollectionObject. Then, a new method needs to be implemented for
> WeakCollectionObject (called possibly sizeOfData?) 

That's right.

> that returns
> `mallocSizeOf(getPrivate())` or `mallocSizeOf(getMap())` maybe.

This needs to return getMap()->shallowSizeOfIncludingThis().  The reason is that the map object contains pointers to other allocations that we need to account of too.

> Also, would you be able to elaborate on the other solution you mentioned
> involving a class hook? Guessing this is some way of calling the correct
> code directly without marching through a if-else-if chain, right? The way I
> could think of to achieve that would be to make addSizeOfExcludingThis a
> virtual method in the JSObject base class. Then have the special cases
> override/extend on that. Not sure of the performance implications of using
> virtual methods though. Is that what you meant by class hook?

We don't tend to use virtual methods much in the JS engine, for performance reasons but also because they add a vtable pointer to all objects.  To change the behaviour of JSObjects we have a table of function pointers (js::Class, see js/public/Class.h) that is accessible via JSObject::getClass().  The suggestion was to add a new function pointer for getting the size of the object.  Maybe that would be best done as a separate bug though.

Comment 7

[jlogandavison]

Attachment: patch.diff

I've added the cases to the the if-else statement.

====

It took me a little while to get to grips with running DMD. It appears that this implementation causes a crash when DMD runs.

I tested using some simple JavaScript to instantiate the WeakMap and WeakSet objects before running DMD via the about:memory page. The following error appeared in the terminal:

> DMD[760] opened /tmp/dmd-1538094875-760.json.gz for writing
> DMD[760] Unknown pointer 0x38

Followed by a stack trace.

I've narrowed it down to the call to |getMap()->shallowSizeOfIncludingThis()| (ie: returning zero here does not cause the crash to occur). Any ideas on what's causing this?

====

Other questions:

I added the |sizeOfExcludingThis()| to |WeakCollecionObject|, the shared parent class of both |WeakMapObject| and |WeakSetObject|, though I've implemented separate cases in |JSObject::addSizeOfExcludingThis()|. Would it be appropriate to simply have one case for |WeakCollectionObject|, or does each class need reporting separately?

Comment 8

[Nicholas Nethercote [inactive]]

It sounds like a MallocSizeOf function is being passed an argument that is not a pointer to a heap allocation.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Any ideas on what's causing this?

getMap() can return null (e.g. I think it will do that if you have a WeakMap that no entries have been added to yet).   Given a crash on a near-zero address (0x38), I would assume that this is what's going on.

Comment 10

[Nicholas Nethercote [inactive]]

Ah, bz's explanation sounds more likely to be true than mine.

Comment 11

[jlogandavison]

Attachment: patch.diff

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)
> getMap() can return null (e.g. I think it will do that if you have a WeakMap
> that no entries have been added to yet).

Ah, okay. That threw me off because the WeakMap/WeakSets in my test JS had been instantiated with some content, but I suppose there are other WeakCollections allocated by the browser elsewhere?

A check for nullptr does avoid the crash. Is returning a size of zero valid in this case?

Comment 12

[Nicholas Nethercote [inactive]]

> A check for nullptr does avoid the crash. Is returning a size of zero valid in this case?

Yes. This is a common idiom in memory reporting code:

> size += p ? p->sizeOfIncludingThis(aMallocSizeOf) : 0;

or this:

> if (p) {
>     size += p->sizeOfIncludingThis(aMallocSizeOf);
> }

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 9013029 [details] [diff] [review]
patch.diff

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

Great, thanks for the patch.

::: js/src/builtin/WeakMapObject.h
@@ +20,5 @@
>    public:
>      ObjectValueMap* getMap() { return static_cast<ObjectValueMap*>(getPrivate()); }
>  
> +    size_t sizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +    {

nit: the brace goes on the previous line for inline method definitions.

@@ +22,5 @@
>  
> +    size_t sizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +    {
> +      ObjectValueMap* map = getMap();
> +      return (map != nullptr) ? map->shallowSizeOfIncludingThis(aMallocSizeOf) : 0;

nit: you can just use |map| as the condition rather than |map != nullptr| here.

::: js/src/vm/JSObject.cpp
@@ +59,4 @@
>  
>  #include "builtin/Boolean-inl.h"
>  #include "builtin/TypedObject-inl.h"
> +#include "builtin/WeakMapObject-inl.h"

I'm not sure why you need WeakMapObject-inl.h rather than just WeakMapobject.h.  It would be better to just include the latter if that works.

Comment 14

[jlogandavison]

Attachment: patch.diff

Thanks. I've addressed your suggestions.

One question regarding the if statement in JSObject. I've implemented separate cases for WeakMapObject and WeakSetObject.

Since these classes both inherit from WeakCollectionObject, would it make more sense to have one case, like:
> else if(is<WeakCollectionObject>()) {
>     ...
> } else ... {

rather than two?

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 9013689 [details] [diff] [review]
patch.diff

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

Thanks for the updated patch!

Yes, that would be better.

Comment 16

[jlogandavison]

(In reply to Jon Coppeard (:jonco) (PTO until 1st October) from comment #15)
> Yes, that would be better.

On second look, that doesn't work. The |is<T>| template requires that |T| has member |class_|. |WeakCollectionObject| doesn't have that field. Shall we stick with the current implementation?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> The |is<T>| template requires that |T| has member |class_|. 

is<T> is specialized for WeakCollectionObject at https://searchfox.org/mozilla-central/rev/6ddb5fb144993fb5de044e2e8d900d7643b98a4d/js/src/builtin/WeakSetObject.h#37-42 so is<WeakCollectionObject>()  should work fine at first glance.

Comment 18

[jlogandavison]

Attachment: patch.diff

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

Thanks, indeed it is. I just needed the correct import.

Comment 19

[BugBot [:suhaib / :marco]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jlogandavison, could you have a look please?

Comment 20

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c07a58deb64d062374168336cfcc98f3807e19b0

Comment 21

[Jon Coppeard (:jonco)]

And backed out again for asan failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8455b646ccf0ff981f37c3bef46a8a6e97be128e

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c07a58deb64d062374168336cfcc98f3807e19b0&selectedJob=235216028

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/8455b646ccf0ff981f37c3bef46a8a6e97be128e

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235221291&repo=mozilla-inbound&lineNumber=11709

[task 2019-03-21T17:24:02.395Z] 17:24:02 INFO - TEST-START | dom/media/webaudio/test/test_ScriptProcessorCollected1.html
[task 2019-03-21T17:24:02.723Z] 17:24:02 INFO - GECKO(2540) | MEMORY STAT | vsize 20973491MB | residentFast 592MB
[task 2019-03-21T17:24:02.739Z] 17:24:02 INFO - TEST-OK | dom/media/webaudio/test/test_ScriptProcessorCollected1.html | took 343ms
[task 2019-03-21T17:24:02.817Z] 17:24:02 INFO - GECKO(2540) | JavaScript error: , line 0: InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable
[task 2019-03-21T17:24:02.844Z] 17:24:02 INFO - TEST-START | dom/media/webaudio/test/test_WebAudioMemoryReporting.html
[task 2019-03-21T17:24:03.047Z] 17:24:03 INFO - GECKO(2540) | =================================================================
[task 2019-03-21T17:24:03.047Z] 17:24:03 ERROR - GECKO(2540) | ==2608==ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x60800005ebd8
[task 2019-03-21T17:24:03.287Z] 17:24:03 INFO - GECKO(2540) | #0 0x55945e3c055d in __interceptor_malloc_usable_size /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:191:3
[task 2019-03-21T17:24:03.790Z] 17:24:03 INFO - GECKO(2540) | #1 0x7f83ab49879b in shallowSizeOfIncludingThis /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HashTable.h:207:12
[task 2019-03-21T17:24:03.792Z] 17:24:03 INFO - GECKO(2540) | #2 0x7f83ab49879b in sizeOfExcludingThis /builds/worker/workspace/build/src/js/src/builtin/WeakMapObject.h:26

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f5d2e1fdf4
Add memory reporting for WeakMap and WeakSet r=jonco

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b8f5d2e1fdf4