Closed
Bug 1159506
Opened 8 years ago
Closed 8 years ago
GC events should use TimeStamp
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jsantell, Assigned: tromey)
References
Details
Attachments
(1 file, 7 obsolete files)
6.99 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
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.
Reporter | ||
Comment 4•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Summary: Markers/TimelineActors should use JS_Now → Markers/TimelineActors should use TimeStamp
Assignee | ||
Updated•8 years ago
|
Attachment #8603035 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
(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
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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)
Reporter | ||
Comment 9•8 years ago
|
||
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+
Reporter | ||
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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)
Reporter | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
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...
Reporter | ||
Comment 15•8 years ago
|
||
Looking into it more, this is fine to land whenever, independently of anything else.
Assignee | ||
Comment 16•8 years ago
|
||
(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 | ||
Updated•8 years ago
|
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•8 years ago
|
||
The patch here requires the patch in bug 1159507.
Depends on: 1159507
Assignee | ||
Comment 18•8 years ago
|
||
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)
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
Note for the next round - Terrence also pointed out that JS_GetCurrentTime is not a very good name.
Comment 21•8 years ago
|
||
Not that I have a better suggestion, mind you!
Assignee | ||
Comment 22•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8609486 -
Attachment is obsolete: true
Assignee | ||
Comment 23•8 years ago
|
||
Updated for the name change.
Attachment #8611417 -
Attachment is obsolete: true
Assignee | ||
Comment 24•8 years ago
|
||
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 25•8 years ago
|
||
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+
Assignee | ||
Comment 26•8 years ago
|
||
Zap the obsolete comment.
Attachment #8621185 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8621210 -
Flags: review+
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6d14b686f9
Assignee | ||
Updated•8 years ago
|
Attachment #8623915 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/35091aff4234
Keywords: checkin-needed
Comment 30•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/35091aff4234
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•