1218078 - Show onload and DOMContentLoaded markers in the netmonitor's frontend

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

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

The UI part of bug 859042.

Comment 1

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

Attachment: v1

Needs tests, adding them in a separate patch. This ended up being a bit more involved than expected.

Tom, please take a quick look at the newly added markers.
Olli, please check out the DOM platform bits.
Jordan, frontend.

Comment 2

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

Attachment: v1

Forgot a qref.

See comment 1.

Comment 3

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

Comment on attachment 8682775 [details] [diff] [review]
v1

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

* What's the advantage of having 3 prefs instead of one indicating a lightweight network version?
* Any reason the network markers are on two different events?
* Need tests, should go devtools/server/tests/browser/browser_markers*

Possibly r+ if I understand the reasons behind the first two questions

::: devtools/client/netmonitor/netmonitor-controller.js
@@ +229,5 @@
>        this.NetworkEventsHandler.connect();
>        deferred.resolve();
>      });
>  
> +    this.timelineFront = new TimelineFront(this._target.client, this._target.form);

We should add a trait to Timeline or root actor indicating whether or not the server supports the "networkMonitorMode" for timeline so we don't spin this up otherwise. Also, on older servers, the networkMonitorMode isn't an option, so we'd still be instrumenting everything, and not even get the relevent DOM events

@@ +250,5 @@
>      this._connection = null;
>      this.client = null;
>      this.tabClient = null;
>      this.webConsoleClient = null;
> +    this.timelineFront = null;

Should stop/destroy here, yeah?

::: devtools/client/netmonitor/netmonitor-view.js
@@ +1968,5 @@
> +      let t = NetMonitorController.NetworkEventsHandler.firstDocumentLoadTimestamp;
> +      let delta = Math.floor((t - this._firstRequestStartedMillis) * aScale);
> +      let [r, g, b, a] = REQUESTS_WATERFALL_LOAD_TICKS_COLOR_RGBA;
> +      view32bit[delta] = (a << 24) | (r << 16) | (g << 8) | b;
> +    }

statementless block scoping *swoon*

::: devtools/server/actors/timeline.js
@@ +96,5 @@
> +    },
> +    "document::Load" : {
> +      type: "document::Load",
> +      marker: Arg(0, "json")
> +    },

Should probably add docs for these two with the other marker docs if they're any different than our normal DOM events (from the perspective of timeline/perf tool, not so much network)

Also, maybe we should add these both to the same exposed event, instead of these being emitted with all the other markers in "markers" events like in perf, we should combine them in a "network-markers" event, unless there's a reason to keep these separate (still use the overhead if for some reason we want one of these markers and not the other)

@@ +155,4 @@
>        withMemory: Option(0, "boolean"),
> +      withoutGCMarkers: Option(0, "boolean"),
> +      withoutFrames: Option(0, "boolean"),
> +      withoutAggregatedMarkerEvents: Option(0, "boolean")

Yuck, anyway to not have negative options here? I'd rather a `onlyDOMStartEvents` boolean that is the same as these 3 options, or something similar, rather than have negative options. And it's a bit ugly but maybe a "networkMonitorMode" or something that smartly filters out all other markers, as a "lightweight" form of the timeline actor

::: devtools/server/performance/timeline.js
@@ +125,5 @@
>      // which lets us preserve tail sharing when transferring the
>      // frames to the client.  We must waive xrays here because Firefox
>      // doesn't understand that the Debugger.Frame object is safe to
>      // use from chrome.  See Tutorial-Alloc-Log-Tree.md.
> +      if (!this._withoutFrames) {

Maybe its the diff, but looks like this indentation is weird.

@@ +193,5 @@
>     *         whether or not a `memory` event gets fired.
> +   * @option {boolean} withoutGCMarkers
> +   *         Boolean indicating whether or not GC markers should be emitted.
> +   *         TODO: Remove these fake GC markers altogether in bug 1198127
> +   * @option {boolean} withoutFrames

Missing withoutAggregatedMarkerEvents docs, but again I'd prefer a unified option for this unless there's a strong reason otherwise

Comment 4

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> Comment on attachment 8682775 [details] [diff] [review]
> v1
> 
> Review of attachment 8682775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> * What's the advantage of having 3 prefs instead of one indicating a
> lightweight network version?

A "netmonitor" mode is too specific. The fact that this actor is used in a timeline tool shouldn't dictate what options it has. Another network monitor frontend could have different needs.

> * Any reason the network markers are on two different events?

Could have a single event. I'm fine either way.

Agree with other comments.

Comment 5

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> Comment on attachment 8682775 [details] [diff] [review]
> v1
> 
> Review of attachment 8682775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> * What's the advantage of having 3 prefs instead of one indicating a
> lightweight network version?
> * Any reason the network markers are on two different events?
> * Need tests, should go devtools/server/tests/browser/browser_markers*
> 
> Possibly r+ if I understand the reasons behind the first two questions
> 
> ::: devtools/client/netmonitor/netmonitor-controller.js
> @@ +229,5 @@
> >        this.NetworkEventsHandler.connect();
> >        deferred.resolve();
> >      });
> >  
> > +    this.timelineFront = new TimelineFront(this._target.client, this._target.form);
> 
> We should add a trait to Timeline or root actor indicating whether or not
> the server supports the "networkMonitorMode" for timeline so we don't spin
> this up otherwise. Also, on older servers, the networkMonitorMode isn't an
> option, so we'd still be instrumenting everything, and not even get the
> relevent DOM events
> 
> @@ +250,5 @@
> >      this._connection = null;
> >      this.client = null;
> >      this.tabClient = null;
> >      this.webConsoleClient = null;
> > +    this.timelineFront = null;
> 
> Should stop/destroy here, yeah?
> 
> ::: devtools/client/netmonitor/netmonitor-view.js
> @@ +1968,5 @@
> > +      let t = NetMonitorController.NetworkEventsHandler.firstDocumentLoadTimestamp;
> > +      let delta = Math.floor((t - this._firstRequestStartedMillis) * aScale);
> > +      let [r, g, b, a] = REQUESTS_WATERFALL_LOAD_TICKS_COLOR_RGBA;
> > +      view32bit[delta] = (a << 24) | (r << 16) | (g << 8) | b;
> > +    }
> 
> statementless block scoping *swoon*
> 
> ::: devtools/server/actors/timeline.js
> @@ +96,5 @@
> > +    },
> > +    "document::Load" : {
> > +      type: "document::Load",
> > +      marker: Arg(0, "json")
> > +    },
> 
> Should probably add docs for these two with the other marker docs if they're
> any different than our normal DOM events (from the perspective of
> timeline/perf tool, not so much network)

They're not different. 

> 
> Also, maybe we should add these both to the same exposed event, instead of
> these being emitted with all the other markers in "markers" events like in
> perf, we should combine them in a "network-markers" event, unless there's a
> reason to keep these separate (still use the overhead if for some reason we
> want one of these markers and not the other)

Emitting them together in the same event is a bad idea. These two markers could be potentially created seconds apart. We want to display them in the frontend as soon as they're created.

Comment 6

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

Attachment: v2

Addressed comments, removed negative options etc. See previous two comments.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8682775 [details] [diff] [review]
v1

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

::: docshell/base/timeline/DocumentLoadTimelineMarker.h
@@ +16,5 @@
> +{
> +public:
> +  explicit DocumentLoadTimelineMarker()
> +    : TimelineMarker("document::Load", MarkerTracingType::TIMESTAMP)
> +    , mUnixTime(PR_Now())

This seems to be a duplicate of the other new class with just a different string constant.
If you expect them to diverge, this is fine; otherwise you might as well just make the constant an argument.

Comment 8

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

Comment on attachment 8683073 [details] [diff] [review]
v2

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

Sorry for all the nits. I think this is all based around my desire to have a lightweight performance actor if desired, spawning from the work in recording with no markers (and also no realtime markers)[0]. I think the main things are:

* All options should be false by default
* s/withMarkerEvents/withMarkers
* A new option for withDocLoadingMarkers, which is a subset of withMarkers, just emitting the network markers. That way, if (!withMarkers && !withDocLoadingMarkers), then the timeline actor is more of an aggregate for its subcomponents, like memory, although that certainly limits its uses, but does make perftools more modular. But we should definitely have an option indicating we want the lightweight markers. Actually, in that case, we can still expose them via `markers`, but have `withMarkers` false, and `withDocLoadingMarkers` true, and the timeline module will only emit the 2 doc loading markers.

Needs a test for this, but can be done in a follow up (assign me).

Sorry for all the nits. If this is too annoying, we can handle the API finesse changes in a follow up, while the network monitor gets a little extra data in the meantime.

[0] https://bugzilla.mozilla.org/showdependencytree.cgi?id=1197031&hide_resolved=1

::: devtools/client/netmonitor/netmonitor-controller.js
@@ +212,5 @@
> +      return deferred.promise;
> +    };
> +
> +    let connectTimeline = options => {
> +      this.timelineFront = new TimelineFront(this._target.client, this._target.form);

What happens if the server doesn't have a timeline actor (~Fx35?)? Since we don't do anything with it I think it'll be ok.

@@ +244,5 @@
>      this.client = null;
>      this.tabClient = null;
>      this.webConsoleClient = null;
> +
> +    yield this.timelineFront.stop();

Check this._target.getTrait("documentLoadingMarkers") here as well before stopping incase timeline actor isn't supported

::: devtools/server/actors/root.js
@@ +177,5 @@
>      // fully equipped to handle heap snapshots for the memory tool. Fx44+
>      heapSnapshots: true,
> +    // Whether or not the timeline actor can emit DOMContentLoaded and Load
> +    // markers, currently in use by the network monitor.
> +    documentLoadingMarkers: true

Specify Fx45+ just for convenience

::: devtools/server/actors/timeline.js
@@ +45,5 @@
>    events: {
>      /**
> +     * Events emitted when "DOMContentLoaded" and "Load" markers are received.
> +     */
> +    "doc-loading" : {

This is what I meant by a single event -- sorry if this wasn't clear -- looks good!

@@ +151,5 @@
> +      withMarkerEvents: Option(0, "boolean"), // default: true
> +      withTicks: Option(0, "boolean"),        // default: false
> +      withMemory: Option(0, "boolean"),       // default: false
> +      withGCMarkers: Option(0, "boolean"),    // default: true
> +      withFrames: Option(0, "boolean"),       // default: true

These should all default to false for consistency -- small change in perf tools for this

::: devtools/server/performance/timeline.js
@@ -68,5 @@
>  
>      events.off(this.tabActor, "window-ready", this._onWindowReady);
>      this.tabActor = null;
> -
> -    if (this._memory) {

This isn't necessary anymore? The destroy method also calls `stop` on the subcomponents, I'd be surprised if this is ok

-- 

oh now handled in `stop`, nevermind

@@ +136,4 @@
>        }
> +
> +      // Emit some helper events for "DOMContentLoaded" and "Load" markers.
> +      switch (marker.name) {

So by default, minimum, these two marker types are always emitted? Maybe an option `withDocLoadingMarkers` for this -- that way if that, and `withMarkerEvents` are false, then we don't need to do any markers at all. The reason I mention this is for the feature of perf recording with no markers at all (for example, just profiling in a lightweight way), which is a stack of a few bugs open still, and the blocker for that is the graphs not handling it.

@@ +222,5 @@
> +    this._withMarkerEvents = withMarkerEvents === undefined || withMarkerEvents !== false;
> +    this._withTicks = !!withTicks;
> +    this._withMemory = !!withMemory;
> +    this._withGCMarkers = withGCMarkers === undefined || withGCMarkers !== false;
> +    this._withFrames = withFrames === undefined || withFrames !== false;

Once all default to false, these checks will just be `!!`

Comment 9

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

Agreed with everything.

Comment 10

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

(In reply to Tom Tromey :tromey from comment #7)
> Comment on attachment 8682775 [details] [diff] [review]
> v1
> 
> Review of attachment 8682775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/timeline/DocumentLoadTimelineMarker.h
> @@ +16,5 @@
> > +{
> > +public:
> > +  explicit DocumentLoadTimelineMarker()
> > +    : TimelineMarker("document::Load", MarkerTracingType::TIMESTAMP)
> > +    , mUnixTime(PR_Now())
> 
> This seems to be a duplicate of the other new class with just a different
> string constant.
> If you expect them to diverge, this is fine; otherwise you might as well
> just make the constant an argument.

If we add the marker name as an argument, I'm wary of people using this class as a general purpose marker that has a unix timestamp. I'd very much want us to avoid that, but maybe making sure such a patch would never pass review could work.

Comment 11

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

Attachment: v3

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8682775 [details] [diff] [review]
v1

>+++ b/docshell/base/timeline/DocumentDOMContentLoadedTimelineMarker.h
>@@ -0,0 +1,40 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_DocumentDOMContentLoadedTimelineMarker_h_
>+#define mozilla_DocumentDOMContentLoadedTimelineMarker_h_
>+
>+#include "TimelineMarker.h"
>+#include "mozilla/dom/ProfileTimelineMarkerBinding.h"
>+
>+namespace mozilla {
>+
>+class DocumentDOMContentLoadedTimelineMarker : public TimelineMarker
A bit long name. Why Document prefix.
I'd have just DOMContentLoadedTimelineMarker for the class and filename

>+{
>+public:
>+  explicit DocumentDOMContentLoadedTimelineMarker()
>+    : TimelineMarker("document::DOMContentLoaded", MarkerTracingType::TIMESTAMP)
though, if this is some convention to have class name similar to marker name, fine, no need to
change the name in that case.


>+#endif // mozilla_DocumentDOMContentLoadedTimelineMarker_h_
>diff --git a/docshell/base/timeline/DocumentLoadTimelineMarker.h b/docshell/base/timeline/DocumentLoadTimelineMarker.h
>new file mode 100644
>--- /dev/null
>+++ b/docshell/base/timeline/DocumentLoadTimelineMarker.h
>@@ -0,0 +1,40 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_DocumentLoadTimelineMarker_h_
>+#define mozilla_DocumentLoadTimelineMarker_h_
>+
>+#include "TimelineMarker.h"
>+#include "mozilla/dom/ProfileTimelineMarkerBinding.h"
>+
>+namespace mozilla {
>+
>+class DocumentLoadTimelineMarker : public TimelineMarker
>+{
>+public:
>+  explicit DocumentLoadTimelineMarker()
>+    : TimelineMarker("document::Load", MarkerTracingType::TIMESTAMP)
>+    , mUnixTime(PR_Now())
>+  {}
>+
>+  virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
>+  {
>+    TimelineMarker::AddDetails(aCx, aMarker);
>+    aMarker.mUnixTime.Construct(mUnixTime);
>+  }
>+
>+private:
>+  // Certain consumers might use Date.now() or similar for tracing time.
>+  // However, TimelineMarkers use process creation as an epoch, which provides
>+  // more precision. To allow syncing, attach an additional unix timestamp.
>+  // Using this instead of `AbstractTimelineMarker::GetTime()'s` timestamp
>+  // is strongly discouraged.
>+  PRTime mUnixTime;
>+};
hmm, why do we have exactly the same class twice, just with different name?

Please create some generic class which takes const char* as param so that the name can be passed.

Comment 13

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

Comment on attachment 8683758 [details] [diff] [review]
v3

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

Looks great! Some last comments below, and I think we'll need to update a few more performance things:

* traits.features in the PerformanceActor
* mapRecordingOptions in devtools/shared/performance/recording-utils
* traits.features in devtools/client/performance/legacy/front (all the new fields should be false and shouldn't affect logic, but for consistency)

I think that perf tests should fail without the PerformanceActor traits.features at the very least

::: devtools/client/netmonitor/netmonitor-controller.js
@@ +242,5 @@
>      this.webConsoleClient = null;
> +
> +    // The timeline front wasn't initialized and started if the server wasn't
> +    // recent enough to emit the markers we were interested in.
> +    if (this._target.getTrait("documentLoadingMarkers")) {

nit: maybe `if (this.timelineFront) {` should be the same logic, but maybe clearer? idk

@@ +527,5 @@
>      dumpn("NetworkEventsHandler is connecting...");
>      this.webConsoleClient.on("networkEvent", this._onNetworkEvent);
>      this.webConsoleClient.on("networkEventUpdate", this._onNetworkEventUpdate);
> +
> +    if (this._target.getTrait("documentLoadingMarkers")) {

Same here, if we have a timeline front, let's assume it has everything we want, and `if (this.timelineFront)` is cleaner/nicer, and someone reading this part of the code doesn't care why a timeline front could exist, just that it does exist

@@ +545,5 @@
> +    dumpn("NetworkEventsHandler is disconnecting...");
> +    this.webConsoleClient.off("networkEvent", this._onNetworkEvent);
> +    this.webConsoleClient.off("networkEventUpdate", this._onNetworkEventUpdate);
> +
> +    if (this._target.getTrait("documentLoadingMarkers")) {

ditto

::: devtools/client/performance/performance-controller.js
@@ +262,4 @@
>        withMemory: this.getOption("enable-memory"),
> +      withFrames: true,
> +      withGCEvents: true,
> +      withDocLoadingEvents: false,

should just omit this property

::: devtools/server/tests/browser/browser_markers-docloading-02.js
@@ +14,5 @@
> +  initDebuggerServer();
> +  let client = new DebuggerClient(DebuggerServer.connectPipe());
> +  let form = yield connectDebuggerClient(client);
> +  let front = TimelineFront(client, form);
> +  let rec = yield front.start({ withMarkers: true, withDocLoadingEvents: true });

I think a better test would be `withMarkers` being false, like the netmonitor has

Comment 14

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

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #13)
> Comment on attachment 8683758 [details] [diff] [review]
> v3
> 
> I think a better test would be `withMarkers` being false, like the
> netmonitor has

Did you look at the other test? It does exactly that.

Comment 15

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

(In reply to Victor Porof [:vporof][:vp] from comment #14)
> (In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from
> comment #13)
> > Comment on attachment 8683758 [details] [diff] [review]
> > v3
> > 
> > I think a better test would be `withMarkers` being false, like the
> > netmonitor has
> 
> Did you look at the other test? It does exactly that.

Oh, scratch that, the other test checks the opposite scenario. I'll add another test.

Comment 16

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

Attachment: v4

Addressed all comments.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/096b5587a7fa

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/096b5587a7fa

Comment 20

[Will Bamberg [:wbamberg]]

I've added a note on this: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Timeline

Comment 21

[Tim Nguyen :ntim]

Release Note Request (optional, but appreciated)
[Why is this notable]: new network monitor feature
[Suggested wording]: DOMContentLoaded and load events are now shown in the network monitor timeline
[Links (documentation, blog post, etc)]:

Comment 22

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

Nice!

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with Tim's wording.