1136945 - Show GCs in the waterfall

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

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

We can either add markers for them (I was almost sure that there already are markers for GC, do we just not render them??) or we can integrate data from the existing GC stats API.

Comment 1

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

Do you know if there are already markers for GCs?

Comment 2

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

:BenWa says that the gecko profiler addon uses the GC stats API and integrates it out of band.

We should do that too, because there is no point in exposing allocation stuff if we can't help users identify that they are in fact janking due to GC pauses.

Comment 3

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

For posterity: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Internals/GC/Statistics_API

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Also: https://github.com/past/memory-profiler/blob/master/chrome/tool.js#L201

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

We have two memory actors in use for performance: one used by the timeline actor, to call `measure()` and get the memory consumption graph, and one used along side the timeline and profiler actor for allocations.

So currently, the GC events are registered once the memory actor is attached. Only the allocation memory actor calls `attach()` and calls addDebuggees(). To have this consumed by the timeline's back end (preferable, mixed in with the other markers, for our use anyway), and since we want this on by default and not require "enable memory" to be set, is there an overhead for setting up addDebuggees for this event? Is it necessary, and could be set up in the constructor, or another method instead?

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Have to do some time normalization with this data, but seems to sync up well with the gaps in JS. Nick, does this accurate? can the incremental GC occur within a JS frame? http://i.imgur.com/J91CXws.png

Also, what's a more interesting thing to show on the waterfall? Right now it's just the "reason", but maybe it should only display the nonincremental reason.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1136945-gc-markers.patch

Nick, check out the memory actor stuff, Victor everything else, but mostly timing conversions stuff. Seems pretty in sync with the JS timing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13963177d57b

Comment 9

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)
> We have two memory actors in use for performance: one used by the timeline
> actor, to call `measure()` and get the memory consumption graph, and one
> used along side the timeline and profiler actor for allocations.

Why use two MemoryActors in the first place? Every time I hear about that, I can't help but feel like it is a horrible bit of cruft.

> So currently, the GC events are registered once the memory actor is
> attached. Only the allocation memory actor calls `attach()` and calls
> addDebuggees(). To have this consumed by the timeline's back end
> (preferable, mixed in with the other markers, for our use anyway), and since
> we want this on by default and not require "enable memory" to be set, is
> there an overhead for setting up addDebuggees for this event? Is it
> necessary, and could be set up in the constructor, or another method instead?

If you don't add debuggees, then you won't observe any GCs because none of your zero debuggees will ever participate in a GC.

Adding debuggees shouldn't induce any performance overhead.

The GCs should always be shown in the waterfall, not only when "enable memory" is checked. You would enable memory/allocations tracking once you see long GC pauses.

Comment 10

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

Comment on attachment 8598477 [details] [diff] [review]
1136945-gc-markers.patch

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

Don't use a MemoryActor that is not exposed over the RDP but instead hidden in another actor for this stuff. Either use the MemoryActor over the RDP, or use the Debugger API directly. The MemoryActor's only job is to expose the Debugger API's memory APIs over the RDP.

I think the whole MemoryActor-within-the-TimelineActor would be much better served by using the Debugger API and sizeOfTab APIs directly. There is no benefit gained from having the actor there.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +185,5 @@
> +
> +        * "GC mode"
> +        * "malloc bytes trigger"
> +        * "allocation trigger"
> +        * "requested"

The changes in this file look good, feel free to pull out to another patch with r=me

::: toolkit/devtools/server/actors/memory.js
@@ +94,5 @@
>      this._dbg = null;
>      this._frameCache = frameCache;
> +    // A new event target is used so events can be shared locally without going
> +    // over RDP, which will throw if there are no listeners.
> +    this.eventTarget = EventTarget();

This removes the RDP events, which is not what we want here (and is going to cause tests to fail).

Comment 11

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> Have to do some time normalization with this data, but seems to sync up well
> with the gaps in JS. Nick, does this accurate? can the incremental GC occur
> within a JS frame? http://i.imgur.com/J91CXws.png
> 
> Also, what's a more interesting thing to show on the waterfall? Right now
> it's just the "reason", but maybe it should only display the nonincremental
> reason.

Yeah, GC can happen within JS. As a hueristic, I also believe that we do some GC work in between frames.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1136945-gc-markers.patch

Revised; pulled out the Debugger.Memory interfaces into a bridge consumed by the MemoryActor, and the TimelineActor now just uses the bridge without creating a new actor.

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1136945-docs.patch

Docs for Debugger.Memory, r=fitzgen

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

try for changes in attachment 8598782 [details] [diff] [review] https://treeherder.mozilla.org/#/jobs?repo=try&revision=00550ed38bf2

Comment 15

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

Comment on attachment 8598782 [details] [diff] [review]
1136945-gc-markers.patch

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

I like this approach a lot better in general.

I don't feel super strongly about it, but I'd probably only put things that are actually shared in the bridge.

::: toolkit/devtools/server/actors/timeline.js
@@ +261,5 @@
>      for (let docShell of this.docShells) {
>        docShell.recordProfileTimelineMarkers = true;
>      }
>  
> +    this._memoryBridge = new MemoryBridge(this.tabActor, this._stackFrames);

This should happen in the constructor, and only the attaching should happen here.

@@ +293,5 @@
>      this._isRecording = false;
>      this._stackFrames = null;
>  
> +    events.off(this._memoryBridge, "garbage-collection", this._onGarbageCollection);
> +    this._memoryBridge = null;

This shouldn't set the _memoryBridge to null, it should detach.

::: toolkit/devtools/server/actors/utils/memory-bridge.js
@@ +44,5 @@
> +/**
> + * A class that returns memory data for a parent actor's window.
> + * A tab-scoped instance of this actor will measure the memory footprint of its
> + * parent tab. A global-scoped instance however, will measure the memory
> + * footprint of the chrome window referenced by the root actor.

This comment needs updating, since this is not an actor.

@@ +53,5 @@
> + */
> +let MemoryBridge = Class({
> +  extends: EventTarget,
> +
> +  initialize: function (parent, frameCache = new StackFrameCache()) {

Please add doc comments explaining these parameters and their expected types.

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 8598782 [details] [diff] [review]
1136945-gc-markers.patch

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

Nothing to add really after Nick's been looking at it several times.

I like how every time I review one of Jordan's patches, there's a new file with bits and pieces from other files. Node peeps would be proud. (I like it).

::: toolkit/devtools/server/actors/memory.js
@@ +7,3 @@
>  let protocol = require("devtools/server/protocol");
>  let { method, RetVal, Arg, types } = protocol;
> +const { MemoryBridge } = require("./utils/memory-bridge");

Nit: let let const.

::: toolkit/devtools/server/actors/timeline.js
@@ +213,2 @@
>      }
>      if (this._framerateActor) {

Nit: change to `this._withTicks` for symmetry.

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8598782 [details] [diff] [review]
1136945-gc-markers.patch

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

Made all the changes -- Nick, let me know if the memory bridge being in the `start` method is a deal breaker for a reason I'm unaware about, but that's the smallest impact area

::: toolkit/devtools/server/actors/timeline.js
@@ +261,5 @@
>      for (let docShell of this.docShells) {
>        docShell.recordProfileTimelineMarkers = true;
>      }
>  
> +    this._memoryBridge = new MemoryBridge(this.tabActor, this._stackFrames);

This needs stack frames, and want to delay the creating of that -- how important is this?

@@ +293,5 @@
>      this._isRecording = false;
>      this._stackFrames = null;
>  
> +    events.off(this._memoryBridge, "garbage-collection", this._onGarbageCollection);
> +    this._memoryBridge = null;

+1

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1136945-gc-markers.patch

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/ef2bdaf2121e
remote:   https://hg.mozilla.org/integration/fx-team/rev/d915bf908fe7
remote:   https://hg.mozilla.org/integration/fx-team/rev/285639a4d08c
remote:   https://hg.mozilla.org/integration/fx-team/rev/757ec9880c5b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef2bdaf2121e
https://hg.mozilla.org/mozilla-central/rev/d915bf908fe7