See bug 1152829 for more info.

Can't include XPCOM in SpiderMonkey :(

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.

(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.

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.

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.

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.

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.

Rebased.

Jordan, I think this one needs front-end changes as well, though I don't know where.

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

This patch undoes the micro-to-milli conversion on the front end for allocations. With this, the allocation time changes can land independently.

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.

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.

This might be somewhat improved by bug 858927, which moves TimeStamp to mfbt.

Update a failing jit test.

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.

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?

(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".

(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.

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.

This required one small test change.

Rebased.