Closed Bug 1159507 Opened 5 years ago Closed 5 years ago

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

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

(2 files, 6 obsolete files)

See bug 1152829 for more info.
Can't include XPCOM in SpiderMonkey :(
Status: NEW → RESOLVED
Closed: 5 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
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
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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 → ---
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.
This version includes a patch to timeline.js to remove the time
normalization.
Attachment #8608869 - Attachment is obsolete: true
Attachment #8608869 - Attachment is obsolete: false
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
Rebased.
Attachment #8608869 - Attachment is obsolete: true
Jordan, I think this one needs front-end changes as well, though I don't know where.
Flags: needinfo?(jsantell)
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)
No longer depends on: 1167800
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: 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+
Blocks: 1159506
This might be somewhat improved by bug 858927, which moves TimeStamp to mfbt.
Update a failing jit test.
Attachment #8609485 - Attachment is obsolete: true
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+
(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.
Rename JS_GetCurrentTime and friends to add "embedder".
Attachment #8616150 - Attachment is obsolete: true
(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.
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+
This required one small test change.
Attachment #8620494 - Attachment is obsolete: true
Attachment #8621586 - Flags: review+
Rebased.
Attachment #8621586 - Attachment is obsolete: true
Attachment #8623669 - Flags: review+
Attachment #8623669 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/08fd89d1427f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.