1149294 - Should be able to run heap snapshot analyses in workers

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

There is no technical reason why ChromeUtils.readHeapSnapshot and the HeapSnapshot interface aren't exposed in workers, but we should be doing heavy graph computations (like dominator trees) off main thread. The MemoryActor in the devtools server should have a worker for this stuff.

Comment 1

[Noemí Freire (:noemi)]

A way to monitor memory consumption from a Service Worker was something discussed as desirable during the Service Workers work week in Madrid, adding the corresponding dependency. Thanks!

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note that because of bug 1173002, we will have to do a work around to expose ChromeUtils and HeapSnapshot to workers. We will need to use a custom exposure function: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Upon further digging, it appears that JS::ubi::RootList is not thread safe.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Don't trace permanent atoms and well known symbols that are owned by a parent JSRuntime inside SimpeEdgeVectorTracer

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: expose ChromeUtils and HeapSnapshot to workers

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8617542 [details] [diff] [review]
Part 2: expose ChromeUtils and HeapSnapshot to workers

Bobby, should I rename ChromeUtils to ThreadSafeChromeUtils as a part of this patch series?

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8617541 [details] [diff] [review]
Part 1: Don't trace permanent atoms and well known symbols that are owned by a parent JSRuntime inside SimpeEdgeVectorTracer

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

This doesn't handle well-known symbols, so that at least needs fixing. Also, using a template doesn't really get you anything here since you don't have the type at all. Also you shouldn't need to check the runtime directly -- just as fast to look at the string/symbol flags. Turns out we have an instance in the tree that's doing basically what you need. Please try something like:

https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Verifier.cpp#289

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Don't trace permanent atoms and well known symbols that are owned by a parent JSRuntime inside SimpeEdgeVectorTracer

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Don't trace permanent atoms and well known symbols that are owned by a parent JSRuntime inside SimpeEdgeVectorTracer

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8617594 [details] [diff] [review]
Part 1: Don't trace permanent atoms and well known symbols that are owned by a parent JSRuntime inside SimpeEdgeVectorTracer

Well, that was a lot simpler.

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8617594 [details] [diff] [review]
Part 1: Don't trace permanent atoms and well known symbols that are owned by a parent JSRuntime inside SimpeEdgeVectorTracer

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

Nice!

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8617542 [details] [diff] [review]
Part 2: expose ChromeUtils and HeapSnapshot to workers

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

This patch should definitely put these methods in ThreadsafeChromeUtils. But we'll also still need a ChromeUtils for non-threadsafe things.

There are other in-flight patches that add non-threadsafe things to ChromeUtils, notably bug 1170097. I'm concerned that, even if this patch does the rename and adds an empty ChromeUtils interface, the other patches are likely to apply in such a way that the methods end up in ThreadsafeChromeUtils. Please find some way to coordinate with that bug such that this doesn't happen.

r=me with that.

::: dom/webidl/HeapSnapshot.webidl
@@ +6,5 @@
>  
>  /**
>   * A HeapSnapshot represents a snapshot of the heap graph
>   */
> +[ChromeOnly, Exposed=(Window,System, Worker)]

Nit - remove the space after the comma

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Create an empty ThreadSafeChromeUtils interface

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Move heap snapshot methods to ThreadSafeChromeUtils and update tests accordingly

Comment 15

[Bobby Holley (:bholley)]

I think all of the implementation methods (threadsafe and not) should live in a file called dom/base/ChromeUtils.cpp.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8620644 [details] [diff] [review]
Part 4: Move heap snapshot methods to ThreadSafeChromeUtils and update tests accordingly

Note that this is pretty much just a mechanical change.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Parts 3 and 4 should make rebasing pretty easy for whoever lands first.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8620643 [details] [diff] [review]
Part 3: Create an empty ThreadSafeChromeUtils interface

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

Per above I don't think this stuff should live in devtools anymore. Move it to dom/base/ChromeUtils.{cpp,h} (for both threadsafe and non-threadsafe).

r=me with that.

::: dom/webidl/ThreadSafeChromeUtils.webidl
@@ +5,5 @@
> + */
> +
> +/**
> + * A collection of **thread-safe** static utility methods that are only exposed
> + * to Chrome.

Mention workers here.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8620644 [details] [diff] [review]
Part 4: Move heap snapshot methods to ThreadSafeChromeUtils and update tests accordingly

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

Canceling review per the above.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Bobby Holley (:bholley) from comment #15)
> I think all of the implementation methods (threadsafe and not) should live
> in a file called dom/base/ChromeUtils.cpp.

But then devtools people would have to get DOM review for everything...

Comment 21

[Bobby Holley (:bholley)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #20)
> (In reply to Bobby Holley (:bholley) from comment #15)
> > I think all of the implementation methods (threadsafe and not) should live
> > in a file called dom/base/ChromeUtils.cpp.
> 
> But then devtools people would have to get DOM review for everything...

Why? If a devtools-y method happens to live in dom/base, it can still be reviewed by a devtools person. Or alternatively, the method can delegate to a helper that lives inside devtools.

ChromeUtils is clearly becoming more general than devtools. It shouldn't live there.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8620665 [details] [diff] [review]
Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Sorry for the bugspam, I forgot to mention workers above the ThreadSafeChromeUtils interface definition in the old patch.

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8620707 [details] [diff] [review]
Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

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

Combines the ChromeUtils.* and ThreadSafeChromeUtils.* into one file. Makes ChromeUtils inherit from ThreadSafeChromeUtils as discussed on IRC.

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: combined.patch

This is a rollup of all the patches, and exhibits the codegen bug where the generated cpp bindings don't include mozilla/dom/ChromeUtils.h and also the following error:

> 0:19.37 In file included from /Users/fitzgen/src/mozilla-central/obj.noindex/dom/bindings/UnifiedBindings2.cpp:338:
> 0:19.37 ./ChromeUtilsBinding.cpp:75:67: error: no member named 'GetProtoObjectHandle' in namespace 'mozilla::dom::ThreadSafeChromeUtilsBinding'
> 0:19.38   JS::Handle<JSObject*> parentProto(ThreadSafeChromeUtilsBinding::GetProtoObjectHandle(aCx, aGlobal));
> 0:19.38                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
> 0:19.38 1 error generated.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8620707 [details] [diff] [review]
Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

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

Main issue to figure out here is why the codegen doesn't work, but might as well fix these issues in the mean time.

::: dom/base/ChromeUtils.cpp
@@ +249,5 @@
>  };
>  
>  // A JS::ubi::BreadthFirst handler that serializes a snapshot of the heap into a
>  // core dump.
> +class MOZ_STACK_CLASS HeapSnapshotHandler

As mentioned on IRC, I think it makes the most sense to have the devtools guts live in devtools, and call those guts more abstractly from ChromeUtils. That will prevent the #include dependencies of ChromeUtils from growing even faster than they need to.

::: dom/bindings/Bindings.conf
@@ +269,5 @@
>  'ChromeUtils': {
>      # The codegen is dumb, and doesn't understand that this interface is only a
>      # collection of static methods, so we have this `concrete: False` hack.
>      'concrete': False,
> +    'headerFile': 'mozilla/dom/ChromeUtils.h',

The headerFile directive shouldn't be necessary I don't think, and it doesn't seem to be helping with your include issue. So I'd drop it for this patch, and let bz figure out what's going on with the codegen.

::: toolkit/devtools/server/tests/unit/test_ReadHeapSnapshot.js
@@ +11,5 @@
>  function run_test() {
>    const filePath = getFilePath("core-dump.tmp", true, true);
>    ok(filePath, "Should get a file path");
>  
> +  ThreadSafeChromeUtils.saveHeapSnapshot(filePath, { globals: [this] });

Given our desired inheritance scheme, these shouldn't need to change, right?

Comment 29

[Bobby Holley (:bholley)]

Looks alright to me, not sure why the codegen isn't working. Let's see if bz can figure it out.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> This is a rollup of all the patches,

This totally doesn't apply on tip, and there is no way it can: it looks like it was created by just concatenating several patches, not as an actual diff, but the concatenation was in the wrong order...

Comment 31

[Boris Zbarsky [:bzbarsky]]

OK, so I semi-appplied the patch by hand (enough to be able to run codegen).  Looks ok to me.  ChromeUtilsBinding.cpp has:

  JS::Handle<JSObject*> constructorProto(ThreadSafeChromeUtilsBinding::GetConstructorObjectHandle(aCx, aGlobal));

so ChromeUtils.__proto__ should be ThreasafeChromeUtils, as expected.

Comment 32

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #31)
> so ChromeUtils.__proto__ should be ThreasafeChromeUtils, as expected.

The issue that fitzgen was flagging you for was compilation failures. But I agree that he should provide a rollup that applies to trunk before asking you to debug them. ;-)

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: combined.patch

Sorry about the mess! I suspect my `hgp` git alias can't handle commit ranges...

This patch should apply cleanly on top of the following commit:

> commit cc90f6649e24d4a8ef0e30b83644fc6c3a4dc7f1
> Author: Kim Moir <[e-mail]>
> Date:   Wed Jun 10 12:07:30 2015 -0400
> 
>     a=me no bug update mozharness.json to a64e1cbfb998

Comment 34

[Boris Zbarsky [:bzbarsky]]

OK, so there are two bugs: an empty non-concrete interface that has a parent still needs to include the impl header, and we have no interface prototype objects for things that have no concrete descendants, so need to not be asking for them in that situation.

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Comment 36

[Bobby Holley (:bholley)]

Comment on attachment 8621859 [details] [diff] [review]
Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

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

This is going to need to be rebased on top of yoshi's landing.

r=me with comments addressed.

::: dom/base/ChromeUtils.h
@@ +28,5 @@
> +                               const nsAString& filePath,
> +                               const HeapSnapshotBoundaries& boundaries,
> +                               ErrorResult& rv);
> +
> +  static already_AddRefed<devtools::HeapSnapshot> ReadHeapSnapshot(GlobalObject& global,

Please add a comment above each of these indicating where it's implemented.

::: toolkit/devtools/server/tests/unit/heap-snapshot-worker.js
@@ +7,5 @@
>    console.log("Starting test.");
>    try {
>      const { filePath } = e.data;
>  
> +    ok(ThreadSafeChromeUtils, "Should have access to ThreadSafeChromeUtils");

Can you check that you don't have access to ChromeUtils while you're at it?

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Comment 38

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8622749 [details] [diff] [review]
Part 3: Split thread-safe methods on ChromeUtils out into a new ThreadSafeChromeUtils interface and move the utils interfaces into dom/base

Rebased on latest m-c, carrying over r+.

And now we wait for bug 1173829.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1696276c07f2

Comment 39

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push is green.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38873310354d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b905fe812fab
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4dca36cb90

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38873310354d
https://hg.mozilla.org/mozilla-central/rev/b905fe812fab
https://hg.mozilla.org/mozilla-central/rev/6e4dca36cb90