Closed Bug 1136945 Opened 5 years ago Closed 4 years ago

Show GCs in the waterfall

Categories

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

x86
macOS
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: jsantell)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Do you know if there are already markers for GCs?
Flags: needinfo?(vporof)
: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.
Flags: needinfo?(vporof)
Depends on: 1137527
Duplicate of this bug: 1021017
Assignee: nobody → 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?
Flags: needinfo?(nfitzgerald)
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.
Attached patch 1136945-gc-markers.patch (obsolete) — Splinter Review
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
Attachment #8598477 - Flags: review?(vporof)
Attachment #8598477 - Flags: review?(nfitzgerald)
(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.
Flags: needinfo?(nfitzgerald)
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).
Attachment #8598477 - Flags: review?(nfitzgerald) → review-
See Also: → 1159308
(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.
Attached patch 1136945-gc-markers.patch (obsolete) — Splinter Review
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.
Attachment #8598477 - Attachment is obsolete: true
Attachment #8598477 - Flags: review?(vporof)
Attachment #8598782 - Flags: review?(vporof)
Attachment #8598782 - Flags: review?(nfitzgerald)
Docs for Debugger.Memory, r=fitzgen
Attachment #8598784 - Flags: review+
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.
Attachment #8598782 - Flags: review?(nfitzgerald) → review+
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.
Attachment #8598782 - Flags: review?(vporof) → review+
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
Attachment #8598782 - Attachment is obsolete: true
Attachment #8599067 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef2bdaf2121e
https://hg.mozilla.org/mozilla-central/rev/d915bf908fe7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.