Add Memory Reporting for IonBuilders

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(8 attachments, 6 obsolete attachments)

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.
Good idea.  We should do this for parse tasks too.
Attachment #8962063 - Attachment is obsolete: true
s/CollectTraceLoggerStateStats/CollectGlobalStats

Generalize this so we can add reporting for other non-runtime data (such as HelperThreads).
Attachment #8968484 - Flags: review?(jorendorff)
ionLazyLinkList data can be a non-trivial amount of heap-classified, so report it.
Attachment #8968486 - Flags: review?(jdemooij)
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)
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+
For memory reporting, having a basic iterator for Fifo is useful for HelperThread state measurements.
Attachment #8968491 - Flags: review?(luke)
Hmm.. I think I need to move part 6a before part 3.
Attachment #8968484 - Flags: review?(jorendorff) → review+
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 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+
Fix access to ionLazyLinkList.

Carrying r=jandem from Comment 7.
Attachment #8968486 - Attachment is obsolete: true
Attachment #8968840 - Flags: review+
Fix off-by-one issue and carrying review from Comment 11.
Attachment #8968491 - Attachment is obsolete: true
Attachment #8968841 - Flags: review+
Fixed debug/const compile issues.
Attachment #8968487 - Attachment is obsolete: true
Attachment #8968487 - Flags: review?(jdemooij)
Attachment #8968842 - Flags: review?(jdemooij)
/me shakes fist at spider_check_style
Attachment #8968842 - Attachment is obsolete: true
Attachment #8968842 - Flags: review?(jdemooij)
Attachment #8968866 - Flags: review?(jdemooij)
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 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 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+
(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.
Address nits from Comment 17 and forwarding r=jandem.
Attachment #8968866 - Attachment is obsolete: true
Attachment #8970253 - Flags: review+
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
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
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/188e7c6e6931
Fix ASAN bustage in Part 2. r=me CLOSED TREE
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
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)
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
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 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+
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf0f720c39d
Part 5b: Fix issue with mem reporting IonBuilder. r=jandem
Keywords: leave-open
Depends on: 1456523
https://hg.mozilla.org/mozilla-central/rev/6bf0f720c39d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Duplicate of this bug: 1156316
You need to log in before you can comment on or make changes to this bug.