Allocation times should use the same epoch as the profiler and other timeline actors

RESOLVED FIXED in Firefox 41

Status

P1
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: jsantell, Assigned: tromey)

Tracking

37 Branch
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

See bug 1152829 for more info.
(Reporter)

Updated

3 years ago
Blocks: 1152829
Can't include XPCOM in SpiderMonkey :(
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Although, I suppose we could take a callback from the embedder... That seems like a pretty horrible hackaround...
If we are going to unify the timestamp format, shouldn't it be something that is easy to get from both spidermonkey and gecko?
We still need to do something about this.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Allocation times should use TimeStamp::ProcessCreation → Allocation times should use the same epoch as the profiler and other timeline actors
(In reply to On vacation until May 17th from comment #3)
> If we are going to unify the timestamp format, shouldn't it be something
> that is easy to get from both spidermonkey and gecko?

We've been having this discussion on the mailing list.
Priority: -- → P1
(Assignee)

Comment 6

3 years ago
We agreed in the end to use PRMJ_Now, and I think the allocation sites already do.
I'm going to close this one on that basis.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

3 years ago
Well, that didn't last long.
Switching the profiler to JS_Now seemed too difficult, as it required changes
all over the platform.  Reopening this because it now seems better to switch
to TimeStamp.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

3 years ago
Created attachment 8608869 [details] [diff] [review]
make allocation times consistent with timeline

Here's a patch to make the allocation times consistent with timeline
and with the profiler (after bug 1159486).

Be warned that this is gross; though on the bright side it is the next
patch that is truly awful.

I haven't tested this yet.
(Assignee)

Comment 9

3 years ago
Created attachment 8609385 [details] [diff] [review]
make allocation times consistent with timeline

This version includes a patch to timeline.js to remove the time
normalization.
Attachment #8608869 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8608869 - Attachment is obsolete: false
(Assignee)

Comment 10

3 years ago
Comment on attachment 8609385 [details] [diff] [review]
make allocation times consistent with timeline

That timeline.js change was for the GC markers, not allocation times.
Attachment #8609385 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Created attachment 8609485 [details] [diff] [review]
make allocation times consistent with timeline

Rebased.
Attachment #8608869 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Jordan, I think this one needs front-end changes as well, though I don't know where.
Flags: needinfo?(jsantell)
(Reporter)

Updated

3 years ago
Depends on: 1167800
Same thing as others, I think bug 1167800 should clear it up so you can safely land any of these and it'll work ok. These patches should add a trait to the root actor[0] defining what epoch is used; once bug 1167800 is done, I'll comment what traits need to be added so it should be minimal

[0]https://github.com/mozilla/gecko-dev/blob/fx-team/toolkit/devtools/server/actors/root.js#L174
Flags: needinfo?(jsantell)
(Reporter)

Updated

3 years ago
No longer depends on: 1167800
Created attachment 8612428 [details] [diff] [review]
1159507-alloc-sync-frontend.patch

This patch undoes the micro-to-milli conversion on the front end for allocations. With this, the allocation time changes can land independently.
Attachment #8612428 - Flags: review?(vporof)
Note the above patch does not have backwards compat with non-millisecond allocations, because there were stability issues in previous geckos due to it, and this removes a fork in the code for the few geckos that it could possibly support.
(Assignee)

Updated

3 years ago
Assignee: nobody → ttromey
Status: REOPENED → ASSIGNED
Comment on attachment 8612428 [details] [diff] [review]
1159507-alloc-sync-frontend.patch

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

::: browser/devtools/performance/modules/logic/recording-model.js
@@ +370,5 @@
>          if (!config.withAllocations) { break; }
>          let [{ sites, timestamps, frames, counts }] = data;
> +
> +        let timeOffset = this._memoryStartTime;
> +        RecordingUtils.offsetTimestamps(timestamps, timeOffset);

No need for a timeOffset variable.
Attachment #8612428 - Flags: review?(vporof) → review+
(Assignee)

Updated

3 years ago
Blocks: 1159506
(Assignee)

Comment 17

3 years ago
This might be somewhat improved by bug 858927, which moves TimeStamp to mfbt.
(Assignee)

Comment 18

3 years ago
Created attachment 8616150 [details] [diff] [review]
make allocation times consistent with timeline

Update a failing jit test.
Attachment #8609485 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Comment on attachment 8616150 [details] [diff] [review]
make allocation times consistent with timeline

This changes the allocation times to use a time source consistent with
other performance tool sources.

I think this should not go in unless bug 1159506 is also finished;
but at the same time that should not prevent review.

One open question is whether this should be done more directly by
first requiring the fix for bug 858927.
Attachment #8616150 - Flags: review?(nfitzgerald)
Comment on attachment 8616150 [details] [diff] [review]
make allocation times consistent with timeline

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

So the arbitrary epoch wouldn't be annoying if we also exposed a function to get the current time as a timestamp. But we don't, so when I imagine myself as a non-perf-tools consumer of this API, I feel kinda sad. I don't really have any suggestion for something better, though. I don't think we should expose yet-another way to get a timestamp on the Debugger API.

Perhaps we could just expand the documentation past "arbitrary epoch" and mention that it is up to SpiderMonkey's embedder, defaults to the epoch, and is process creation time in Firefox? Is that too much info / muddying the waters?
Attachment #8616150 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 21

3 years ago
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #20)
> Comment on attachment 8616150 [details] [diff] [review]
> make allocation times consistent with timeline
> 
> Review of attachment 8616150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So the arbitrary epoch wouldn't be annoying if we also exposed a function to
> get the current time as a timestamp. But we don't, so when I imagine myself
> as a non-perf-tools consumer of this API, I feel kinda sad. I don't really
> have any suggestion for something better, though. I don't think we should
> expose yet-another way to get a timestamp on the Debugger API.

Yeah, that is unfortunate.  I did not know what to do about it, though.

> Perhaps we could just expand the documentation past "arbitrary epoch" and
> mention that it is up to SpiderMonkey's embedder, defaults to the epoch, and
> is process creation time in Firefox? Is that too much info / muddying the
> waters?

It seems to me that a comment like that is susceptible to rot; whereas there
will be so few calls to JS_SetCurrentTimeFunction that it isn't a big deal
to search for them all.

I wasn't sure whether I needed another review for the bits outside of SpiderMonkey.
(Assignee)

Comment 22

3 years ago
Created attachment 8620494 [details] [diff] [review]
make allocation times consistent with timeline

Rename JS_GetCurrentTime and friends to add "embedder".
Attachment #8616150 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #20)

> Perhaps we could just expand the documentation past "arbitrary epoch" and
> mention that it is up to SpiderMonkey's embedder, defaults to the epoch, and
> is process creation time in Firefox? Is that too much info / muddying the
> waters?

We talked some more and now there is bug 1173354, to use TimeStamp directly
in SpiderMonkey.  As part of this, I'll rename this function once again, and
update the comment to be more specific about the epoch.  So, I think in the
short term there is nothing to do here.
(Assignee)

Comment 25

3 years ago
Comment on attachment 8620494 [details] [diff] [review]
make allocation times consistent with timeline

I think I need one more review, for the XPCOMInit.cpp change.
Attachment #8620494 - Flags: review?(continuation)
Comment on attachment 8620494 [details] [diff] [review]
make allocation times consistent with timeline

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

I'm not an XPCOM peer, but this looks simple enough. r=me for the XPCOMInit.cpp changes.
Attachment #8620494 - Flags: review?(continuation) → review+
(Assignee)

Comment 27

3 years ago
Created attachment 8621586 [details] [diff] [review]
make allocation times consistent with timeline

This required one small test change.
Attachment #8620494 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8621586 - Flags: review+
(Assignee)

Comment 30

3 years ago
Created attachment 8623669 [details] [diff] [review]
make allocation times consistent with timeline

Rebased.
Attachment #8621586 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8623669 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8623669 - Flags: checkin?

Updated

3 years ago
Attachment #8623669 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/08fd89d1427f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.