Closed
Bug 1448563
Opened 6 years ago
Closed 6 years ago
Add Memory Reporting for IonBuilders
Categories
(Core :: JavaScript Engine, enhancement, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: tcampbell, Assigned: tcampbell)
References
Details
Attachments
(8 files, 6 obsolete files)
9.11 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
7.15 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
3.42 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
7.85 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
7.40 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
1.92 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
10.95 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
IonBuilder is a large structure that can sit in the ZoneGroup lazy-link list, or the HelperThreadState queues. They easily consume 10's of MB which are currently heap-unclassified. Attached is a prototype that adds reporters for some of this data. It can be applied locally to take measurements, but it needs a cleanup to be land-able. This doesn't look at data actively being run by HelperThreads.
Comment 1•6 years ago
|
||
Good idea. We should do this for parse tasks too.
Assignee | ||
Updated•6 years ago
|
Attachment #8962063 -
Attachment is obsolete: true
Assignee | ||
Comment 2•6 years ago
|
||
s/CollectTraceLoggerStateStats/CollectGlobalStats Generalize this so we can add reporting for other non-runtime data (such as HelperThreads).
Attachment #8968484 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•6 years ago
|
||
ionLazyLinkList data can be a non-trivial amount of heap-classified, so report it.
Attachment #8968486 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•6 years ago
|
||
Memory reporting for HelperThreadState. This accounts for work items that are at rest in the global state. NOTE: We don't report memory for things that are currently running on a thread. (Maybe there is a clean way to do this?)
Attachment #8968487 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8968488 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8968489 -
Flags: review?(jdemooij)
Comment 7•6 years ago
|
||
Comment on attachment 8968486 [details] [diff] [review] 0002-Bug-1448563-Part-2-Add-memory-reporting-for-Ion-lazy.patch Review of attachment 8968486 [details] [diff] [review]: ----------------------------------------------------------------- Great. ::: js/src/jit/IonBuilder.h @@ +1187,5 @@ > void trackInlineSuccessUnchecked(InliningStatus status); > + > + public: > + > + // These are only valid for IonBuilder that have moved to background Nit: IonBuilders
Attachment #8968486 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 8•6 years ago
|
||
For memory reporting, having a basic iterator for Fifo is useful for HelperThread state measurements.
Attachment #8968491 -
Flags: review?(luke)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8968492 -
Flags: review?(luke)
Assignee | ||
Comment 10•6 years ago
|
||
Hmm.. I think I need to move part 6a before part 3.
Updated•6 years ago
|
Attachment #8968484 -
Flags: review?(jorendorff) → review+
Comment 11•6 years ago
|
||
Comment on attachment 8968491 [details] [diff] [review] 0006-Bug-1448563-Part-6a-Add-iterator-to-js-Fifo.patch Review of attachment 8968491 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ds/Fifo.h @@ +102,5 @@ > + > + const T& operator*() const { > + // Iterate front in reverse, then rear. > + size_t split = self_.front_.length(); > + return (idx_ < split) ? self_.front_[split - idx_] Doesn't this need to be offset by 1 (so when, e.g., idx_ = 0, we don't access self_.front_[self_.front_.length()) ?
Attachment #8968491 -
Flags: review?(luke) → review+
Comment 12•6 years ago
|
||
Comment on attachment 8968492 [details] [diff] [review] 0007-Bug-1448563-Part-6b-Add-memory-reporting-for-off-thr.patch Review of attachment 8968492 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8968492 -
Flags: review?(luke) → review+
Assignee | ||
Comment 13•6 years ago
|
||
Fix access to ionLazyLinkList. Carrying r=jandem from Comment 7.
Attachment #8968486 -
Attachment is obsolete: true
Attachment #8968840 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
Fix off-by-one issue and carrying review from Comment 11.
Attachment #8968491 -
Attachment is obsolete: true
Attachment #8968841 -
Flags: review+
Assignee | ||
Comment 15•6 years ago
|
||
Fixed debug/const compile issues.
Attachment #8968487 -
Attachment is obsolete: true
Attachment #8968487 -
Flags: review?(jdemooij)
Attachment #8968842 -
Flags: review?(jdemooij)
Assignee | ||
Comment 16•6 years ago
|
||
/me shakes fist at spider_check_style
Attachment #8968842 -
Attachment is obsolete: true
Attachment #8968842 -
Flags: review?(jdemooij)
Attachment #8968866 -
Flags: review?(jdemooij)
Comment 17•6 years ago
|
||
Comment on attachment 8968866 [details] [diff] [review] Bug-1448563-Part-3b-Add-memory-reporting-for-JS-help.patch Review of attachment 8968866 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/HelperThreads.cpp @@ +1122,5 @@ > > +void > +GlobalHelperThreadState::addSizeOfIncludingThis(JS::GlobalStats* stats) const > +{ > + MOZ_ASSERT(isLockedByCurrentThread()); As an alternative, we often pass a reference to AutoLockHelperThreadState: https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/js/src/vm/HelperThreads.h#192 ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +2311,5 @@ > + // Report the number of HelperThread > + > + REPORT(NS_LITERAL_CSTRING("js-helper-threads/idle"), > + KIND_OTHER, UNITS_COUNT, gStats.helperThread.idleThreadCount, > + "The current number of idle JS HelperThreads."); Just to make sure, we report this only once per process (not once-per-runtime), right?
Attachment #8968866 -
Flags: review?(jdemooij) → review+
Comment 18•6 years ago
|
||
Comment on attachment 8968488 [details] [diff] [review] 0004-Bug-1448563-Part-4-Add-memory-reporting-for-off-thre.patch Review of attachment 8968488 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment below addressed. ::: js/src/vm/HelperThreads.cpp @@ +463,5 @@ > +ParseTask::sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const > +{ > + return options.sizeOfExcludingThis(mallocSizeOf) + > + alloc.sizeOfExcludingThis(mallocSizeOf) + > + errors.sizeOfExcludingThis(mallocSizeOf); What about the scripts and sourceObjects Vectors?
Attachment #8968488 -
Flags: review?(jdemooij) → review+
Comment 19•6 years ago
|
||
Comment on attachment 8968489 [details] [diff] [review] 0005-Bug-1448563-Part-5-Add-memory-reporting-for-Ion-offt.patch Review of attachment 8968489 [details] [diff] [review]: ----------------------------------------------------------------- Excellent.
Attachment #8968489 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #18) > Comment on attachment 8968488 [details] [diff] [review] > 0004-Bug-1448563-Part-4-Add-memory-reporting-for-off-thre.patch > > Review of attachment 8968488 [details] [diff] [review]: > ----------------------------------------------------------------- > What about the scripts and sourceObjects Vectors? |scripts| and |sourceObjects| use TempAllocPolicy now which should be accounted elsewhere in a followup bug.
Assignee | ||
Comment 21•6 years ago
|
||
Address nits from Comment 17 and forwarding r=jandem.
Attachment #8968866 -
Attachment is obsolete: true
Attachment #8970253 -
Flags: review+
Comment 22•6 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d19e729f3370 Part 1: Use JS::CollectGlobalReports for non-runtime memory. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/8f922357b90d Part 2: Add memory reporting for Ion lazy linking. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3b241f3634 Part 3a: Add iterator to js::Fifo. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf8b356100c Part 3b: Add memory reporting for JS helper threads. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/d873c3d7b7bf Part 4: Add memory reporting for off-thread parse. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/aad3967f2c7b Part 5: Add memory reporting for Ion offthread. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/97dbd15798f9 Part 6: Add memory reporting for off-thread WASM. r=luke
Comment 23•6 years ago
|
||
Backout by btara@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/99fa845fe022 Backed out changeset 97dbd15798f9 for bad-malloc_usable_size /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc a=backout CLOSED TREE
Comment 24•6 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/188e7c6e6931 Fix ASAN bustage in Part 2. r=me CLOSED TREE
Assignee | ||
Comment 25•6 years ago
|
||
Comment 23 accidentally only backed out Part 6. Will reland once tree is open. Comment 24 fixes part 2 to use exclusive size for IonBuilder. The IonBuilder itself is never system allocated (only LifoAlloc or stack).
Keywords: leave-open
Assignee | ||
Comment 26•6 years ago
|
||
IonBuilder is never on system heap so we should be using exclusive size instead. I had this in an earlier patch but messed up in the final =\
Attachment #8970343 -
Flags: review?(jdemooij)
Comment 27•6 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9938026038b1 Part 6: Add memory reporting for off-thread WASM. r=luke
Assignee | ||
Comment 28•6 years ago
|
||
Comment on attachment 8970343 [details] [diff] [review] Bug-1448563-Part-5b-Fix-issue-with-mem-reporting-Ion.patch This should be named part 5b.
Attachment #8970343 -
Attachment description: Bug-1448563-Part-6b-Fix-issue-with-mem-reporting-Ion.patch → Bug-1448563-Part-5b-Fix-issue-with-mem-reporting-Ion.patch
Comment 29•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d19e729f3370 https://hg.mozilla.org/mozilla-central/rev/8f922357b90d https://hg.mozilla.org/mozilla-central/rev/0d3b241f3634 https://hg.mozilla.org/mozilla-central/rev/7cf8b356100c https://hg.mozilla.org/mozilla-central/rev/d873c3d7b7bf https://hg.mozilla.org/mozilla-central/rev/aad3967f2c7b https://hg.mozilla.org/mozilla-central/rev/188e7c6e6931
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9938026038b1
Comment 31•6 years ago
|
||
Comment on attachment 8970343 [details] [diff] [review] Bug-1448563-Part-5b-Fix-issue-with-mem-reporting-Ion.patch Review of attachment 8970343 [details] [diff] [review]: ----------------------------------------------------------------- Oh right, I should have seen this too.
Attachment #8970343 -
Flags: review?(jdemooij) → review+
Comment 32•6 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf0f720c39d Part 5b: Fix issue with mem reporting IonBuilder. r=jandem
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6bf0f720c39d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•