1145247 - Easy way to instrument platform with TimelineMarkers

Status: RESOLVED FIXED

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

Description

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

Need a way for platform developers to easily add TimelineMarkers

Fitzgen suggested: Simple RAII class to add start/end markers automatically?

We'll also need to add probably a "Category" property, as well as a way to store additional metadata. Currently, TimelineMarkers have a name ("ConsoleTime", "Paint", "DOMEvent"), and optionally a stack, and sometimes a "cause" (the label in console.time/timeEnd).

This should handle both events with one timestamp (maybe implemented in bug 922221), as well as a start and end time.

Comment 1

[Tom Tromey :tromey]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)
> Need a way for platform developers to easily add TimelineMarkers
> 
> Fitzgen suggested: Simple RAII class to add start/end markers automatically?

It's a good idea but was met unfavorably when I tried it last year.
But I think it is worth another try.

One thing to note is that if the two markers are created in the same tick
(which is the case for most existing markers), then we could have this RAII
helper automatically link the start and end markers together, and optimize
PopProfileTimelineMarkers a bit.

> We'll also need to add probably a "Category" property, as well as a way to
> store additional metadata. Currently, TimelineMarkers have a name
> ("ConsoleTime", "Paint", "DOMEvent"), and optionally a stack, and sometimes
> a "cause" (the label in console.time/timeEnd).

I'll comment separately on the category bug.

Note that any given caller can subclass TimelineMarker and add any sort of data
that is relevant.  E.g., this is how Paint markers add the rectangle information.

If new data is added, all that is needed is an addition to ProfileTimelineMarker.webidl,
plus of course a bit of rendering support in marker-details.js.

Comment 2

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

Attachment: Add AutoTimelineMarker RAII class

Comment 3

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

Decided to just go ahead and make the RAII class before we have proper categories and all that because it makes my life easier in other bugs.

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8588908 [details] [diff] [review]
Add AutoTimelineMarker RAII class

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

I'm in favor of it.  While working on the event patch, at one point I had
unpaired start/end markers and didn't notice... this approach prevents that sort
of bug.

One minor thing is whether it makes sense to convert some existing calls to
use this class.  There are two spots in the tree that can use it directly.

Another question is whether it makes sense to handle TimelineMarker subclasses
using this class.  I think subclasses will be the most typical way to interact
with the timeline feature, because it's generally best to send along some
extra data associated with a marker.

Comment 5

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

(In reply to Tom Tromey :tromey from comment #4)
> One minor thing is whether it makes sense to convert some existing calls to
> use this class.  There are two spots in the tree that can use it directly.

Wherever it makes sense, we should (which is kind of tautological). I wouldn't bend over backwards, but if the start and end markers are in the same scope already, then I think it is worth it so we don't have to remember to end at every return.

> Another question is whether it makes sense to handle TimelineMarker
> subclasses
> using this class.  I think subclasses will be the most typical way to
> interact
> with the timeline feature, because it's generally best to send along some
> extra data associated with a marker.

So I started with a template that took the derived TimelineMarker and forwarded arguments to to it in the ctor, but then ran into the issue of when the start and end take a different set of parameters, and then realized I didn't know if that was even an issue or not, and then realized I was way too deep in theoretical issues and that I wasn't even going to use TimelineMarker subclasses for the HTML parsing markers. So I just backed up and did the simplest thing possible.

So, I think it would make sense to support derived classes, but I'm not sure exactly what it would look like, and I feel like it might involve more changes to the markers themselves as well.

Comment 6

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

Comment on attachment 8588908 [details] [diff] [review]
Add AutoTimelineMarker RAII class

I'm worried about not keeping mDocShell alive long enough.
So, nsRefPtr<nsDocShell> but only assign anything to that member variable if 
nsDocShell::gProfileTimelineRecordingsCount > 0

Comment 7

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

Attachment: Add AutoTimelineMarker RAII class

Comment 8

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

Comment on attachment 8589335 [details] [diff] [review]
Add AutoTimelineMarker RAII class

>+  DocShellIsRecording(nsDocShell& docShell)
aDocShell


>+public:
>+  explicit AutoTimelineMarker(nsIDocShell* aDocShell, const char* aName
>+                              MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
>+    : mDocShell(nullptr)
>+    , mName(aName)
>+  {
>+    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>+
>+    nsRefPtr<nsDocShell> docShell = static_cast<nsDocShell*>(aDocShell);
nsRefPtr does slow virtual Addref/Release, so you need to first check if we're recording, and then
just assing docShell to mDocShell.
(docShell doesn't need to be nsRefPtr here, only mDocShell)


>+    if (docShell && DocShellIsRecording(*docShell)) {
>+      mDocShell = Move(docShell);
(.swap() tends to be easier to read than Move(), but you don't need either here, if docShell is just DocShell*)

Comment 9

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

Attachment: Add AutoTimelineMarker RAII class

Comment 10

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/47b14c9bfbcb

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47b14c9bfbcb