We don't seem to have memory reporting for WeakMap hashtables
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jlogandavison, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: [overhead:noted])
Attachments
(1 file, 3 obsolete files)
1.85 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
Hi there, I'm interested in picking up this bug. Is it available?
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 7•6 years ago
|
||
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•6 years ago
|
||
It sounds like a MallocSizeOf function is being passed an argument that is not a pointer to a heap allocation.
Reporter | ||
Comment 9•6 years ago
|
||
> 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•6 years ago
|
||
Ah, bz's explanation sounds more likely to be true than mine.
Assignee | ||
Comment 11•6 years ago
|
||
(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•6 years ago
|
||
> 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•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
(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?
Reporter | ||
Comment 17•6 years ago
|
||
> 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.
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17) Thanks, indeed it is. I just needed the correct import.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 19•5 years ago
|
||
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•5 years ago
|
||
Comment 21•5 years ago
|
||
And backed out again for asan failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8455b646ccf0ff981f37c3bef46a8a6e97be128e
Comment 22•5 years ago
|
||
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•5 years ago
|
||
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f5d2e1fdf4 Add memory reporting for WeakMap and WeakSet r=jonco
Comment 24•5 years ago
|
||
bugherder |
Description
•