Closed Bug 1159506 Opened 5 years ago Closed 5 years ago

GC events should use TimeStamp

Categories

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

37 Branch
defect

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jsantell, Assigned: tromey)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Priority: -- → P1
In bug 1152829 we agreed to use PRMJ_Now (aka JS_Now) for all the actors.
Summary: Markers/TimelineActors should use TimeStamp::ProcessCreation → Markers/TimelineActors should 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.
Minor fixup.
Attachment #8602892 - Attachment is obsolete: true
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?
Summary: Markers/TimelineActors should use JS_Now → Markers/TimelineActors should use TimeStamp
Attachment #8603035 - Attachment is obsolete: true
(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.
Summary: Markers/TimelineActors should use TimeStamp → GC events should use TimeStamp
Attached patch make GC events use TimeStamp (obsolete) — Splinter Review
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.
Attached patch make GC events use TimeStamp (obsolete) — Splinter Review
Rebased.
Attachment #8609400 - Attachment is obsolete: true
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.
Attachment #8609486 - Flags: feedback?(jsantell)
Depends on: 1167800
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?
Attachment #8609486 - Flags: feedback?(jsantell) → feedback+
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
No longer depends on: 1167800
Flags: needinfo?(vporof)
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.
Flags: needinfo?(vporof)
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
Attached patch make GC events use TimeStamp (obsolete) — Splinter Review
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.
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...
Looking into it more, this is fine to land whenever, independently of anything else.
(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.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
The patch here requires the patch in bug 1159507.
Depends on: 1159507
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.
Flags: needinfo?(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.
Flags: needinfo?(terrence)
Note for the next round - Terrence also pointed out that JS_GetCurrentTime is not
a very good name.
Not that I have a better suggestion, mind you!
Blocks: 1173354
(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.
Attachment #8609486 - Attachment is obsolete: true
Attached patch make GC events use TimeStamp (obsolete) — Splinter Review
Updated for the name change.
Attachment #8611417 - Attachment is obsolete: true
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.
Attachment #8621185 - Flags: review?(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?
Attachment #8621185 - Flags: review?(terrence) → review+
Attached patch make GC events use TimeStamp (obsolete) — Splinter Review
Zap the obsolete comment.
Attachment #8621185 - Attachment is obsolete: true
Attachment #8621210 - Flags: review+
Rebased.
Attachment #8621210 - Attachment is obsolete: true
Attachment #8623915 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35091aff4234
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.