1159506 - GC events should use TimeStamp

Status: RESOLVED FIXED

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

Description

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

In order to get consistent times across performance tools' sources, we're going to be using TimeStamp::ProcessCreation to keep things uniform.

We'll need to change the times on markers to use this, as well as tag the memory events from the TimelineActor with the same time system.

Comment 1

[Tom Tromey :tromey]

In bug 1152829 we agreed to use PRMJ_Now (aka JS_Now) for all the actors.

Comment 2

[Tom Tromey :tromey]

Attachment: change timeline markers to use JS_Now

Here's a quick patch to change timeline markers to use JS_Now.

Note that this is just the platform side.  A bit of work is needed on
the client side because the time units have changed from milli- to
micro-seconds.

Comment 3

[Tom Tromey :tromey]

Attachment: change timeline markers to use JS_Now

Minor fixup.

Comment 4

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

There are GC markers emitted by the timeline marker that comes from Debugger.Memory, using times from there (micro unix timestamps) that will need to use this same epoch -- possibly handled in bug 1159507, with allocation site stamps, instead of here?

Comment 5

[Tom Tromey :tromey]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> There are GC markers emitted by the timeline marker that comes from
> Debugger.Memory, using times from there (micro unix timestamps) that will
> need to use this same epoch -- possibly handled in bug 1159507, with
> allocation site stamps, instead of here?

I think a separate bug is good, and since markers are already ok in the new
scheme, let's just mutate this bug.

Comment 6

[Tom Tromey :tromey]

Attachment: make GC events use TimeStamp

Changes GC events to use TimeStamp.  This requires the patch from bug 1159507.

This includes a change to timeline.js to drop the time normalization
step.

Comment 7

[Tom Tromey :tromey]

Attachment: make GC events use TimeStamp

Rebased.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8609486 [details] [diff] [review]
make GC events use TimeStamp

The front end changes could use a look-over, not just for correctness but also to make sure I didn't miss a spot.

Comment 9

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

Comment on attachment 8609486 [details] [diff] [review]
make GC events use TimeStamp

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

This looks good -- no parent process side changes needed. This will work if docShells[0].now() returns the common-epoch used elsewhere -- is that the case already? So all the timeline markers are already good to go?

Comment 10

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

This does not depend on the front end offset massaging, although timeline markers do that currently[0]. If all of timeline markers are using the new timestamp, then we can make a new bug that adds a trait to the root actor (indicating timeline markers are using new format), and handles the case where it needs to offset based off of start time.

Victor, with all this time syncing stuff, what is the epoch expected throughout the widgets? Can we use an arbitrary unix timestamp (take the earliest value from all sources), or do we need to zero-out all these sources anyway?

[0] https://github.com/mozilla/gecko-dev/blob/fx-team/browser/devtools/performance/modules/logic/recording-model.js#L336

Comment 11

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

The epoch should be the start of a recording. This delta should be subtracted only for showing things nicely in the UI, not for syncing.

Comment 12

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

So not worried abour overhead of having to normalize all data to 0? I think its trivial overhead last time I measured, but just want to doublecheck

Comment 13

[Tom Tromey :tromey]

Attachment: make GC events use TimeStamp

The earlier patch is on the invasive side.

At the cost of a bit of ugliness -- duplicating a couple of fields in
Statistics::SliceData, this one is much smaller.

Comment 14

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

This will also require changes to the documentation, right?

Also, is there a bug for making the allocations log use TimeStamp, too? It would be *very* unfortunate to have the Debugger API exposing two different kinds of timestamps...

Comment 15

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

Looking into it more, this is fine to land whenever, independently of anything else.

Comment 16

[Tom Tromey :tromey]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #14)
> This will also require changes to the documentation, right?

Yeah.  I'll put it in the allocations patch.

Comment 17

[Tom Tromey :tromey]

The patch here requires the patch in bug 1159507.

Comment 18

[Tom Tromey :tromey]

Terrence, could you say whether you would prefer the "invasive" approach or the
minimal (but member-duplicating) approach?  I am preparing this for r? but a bit
of earlier guidance could help.  Both patches (the invasive one perhaps somewhat
out of date) are attached here.

To recap the issue is that we would like a single notion of time for all the data
sources feeding the performance tool.  Due to the way the profiler hooks into the
platform, this pretty much has to be based on TimeStamp; see the various related
bugs for details and history if needed.

Comment 19

[Terrence Cole [:terrence]]

I'd much prefer that we put TimeStamp into mozglue and push TimeStamp& through the interface; that would seem to be a much less error-prone change than int64->double. If you need this urgently though, the short patch would be fine as long as we go back to do a real fix at some point.

Comment 20

[Tom Tromey :tromey]

Note for the next round - Terrence also pointed out that JS_GetCurrentTime is not
a very good name.

Comment 21

[Terrence Cole [:terrence]]

Not that I have a better suggestion, mind you!

Comment 22

[Tom Tromey :tromey]

(In reply to Terrence Cole [:terrence] from comment #19)
> I'd much prefer that we put TimeStamp into mozglue and push TimeStamp&
> through the interface; that would seem to be a much less error-prone change
> than int64->double. If you need this urgently though, the short patch would
> be fine as long as we go back to do a real fix at some point.

After discussing with Jordan, I think we want something to go in relatively
soon.  So, I plan to press forward with the hack.  I filed bug 1173354 as a followup
and assigned it to myself; I'll implement it once the TimeStamp move is complete.

Comment 23

[Tom Tromey :tromey]

Attachment: make GC events use TimeStamp

Updated for the name change.

Comment 24

[Tom Tromey :tromey]

Comment on attachment 8621185 [details] [diff] [review]
make GC events use TimeStamp

I'd like to put in this hacky approach and then do the rewrite
using TimeStamp as we discussed.

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 8621185 [details] [diff] [review]
make GC events use TimeStamp

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

::: toolkit/devtools/shared/timeline.js
@@ +271,5 @@
>        return {
>          name: "GarbageCollection",
>          causeName: reason,
>          nonincrementalReason: nonincrementalReason,
>          // Both timestamps are in microseconds -- convert to milliseconds to match other markers

This comment is wrong now?

Comment 26

[Tom Tromey :tromey]

Attachment: make GC events use TimeStamp

Zap the obsolete comment.

Comment 27

[Tom Tromey :tromey]

Attachment: make GC events use TimeStamp

Rebased.

Comment 28

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6d14b686f9

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/35091aff4234

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35091aff4234