1313420 - Implement Performance.timeOrigin

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: timeOrigin.patch

Comment 2

[Andrea Marchesini [:baku]]

I'll add a test in a separate patch.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  This isn't quite right, I think.

As as simple example, consider the shared worker case.  In the shared worker case, mNowBaseTimeHighRes is set via PR_Now() at worker creation time.  So it's not using the monotonic clock at all.

What we really want here, I think, is a single process-wide mozilla::TimeStamp and the corresponding PR_Now() value.  And then we us that one snapshotted PR_Now() value for all the other TimeOrigin() determinations.  At least that's how the spec defines this.

timeOrigin should probbaly be set [Constant] in the IDL, right?

Once we get this checked in, we probably want a followup to stop using the parent's now base time in child workers, using the creation time instead like the spec says, and people can then use timeOrigin to convert.  Separate bug on that.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsIXULAppInfo

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebIDL

Comment 6

[Andrea Marchesini [:baku]]

This approach is based on mozilla::TimeStamp. There is 1 unique startupTimeStamp, shared with the child processes (so that the start time is in sync). Then I use that TimeStamp for calculating timeOrigin (part 2).
Maybe, 'startup' is the wrong word.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebIDL

Comment 8

[Boris Zbarsky [:bzbarsky]]

> There is 1 unique startupTimeStamp, shared with the child processes (so that the start time is in sync).

Does sending a TimeStamp to a different process even make sense?  What ensures they're using the same monotonic clock?  Does all the static state set up on windows end up looking the same across processes?

In any case, this is still wrong, because you want to add the unix time of the start timestamp to the whole thing.  Otherwise you're leaking the browser startup time to the web.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - timeOrigin

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - tests

Comment 11

[Boris Zbarsky [:bzbarsky]]

The part 1 diff is missing the new files....

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - timeOrigin

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - tests

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811166 [details] [diff] [review]
part 1 - timeOrigin

> +static PerformanceService* gPerformanceService = nullptr;

Doesn't this mean that we'll keep destroying and recreating it, in general?  That doesn't look right.

>+PerformanceService::TimeOrigin(const TimeStamp& aCreationTimeStamp) const
>+  MOZ_ASSERT(aCreationTimeStamp > mCreationTimeStamp);

What guarantees this?  I don't think anything actually does.  We can have things getting created before they create their performance objects...

>+  PRTime mCreationEpocTime;

"Epoch", not "Epoc".

Comment 15

[Andrea Marchesini [:baku]]

> Doesn't this mean that we'll keep destroying and recreating it, in general? 

No. It means that the object is destroyed when there are no Performance objects alive. Performance objects keep this PerformanceService alive. This seems correct to me.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 1 - timeOrigin

All the other comments are applied.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> This seems correct to me.

It's not, because it means things will keep resynchronizing to a possibly-changing PR_Now value.  That's explicitly _not_ supposed to happen.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 1 - timeOrigin

Ok, I see what you mean. Here a new version.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811480 [details] [diff] [review]
part 1 - timeOrigin

This works, but now we have to lock on every timeOrigin get, which could become somewhat perf-sensitive... :(

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 1 - timeOrigin

A bit better.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811511 [details] [diff] [review]
part 1 - timeOrigin

r=me

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0f72f0b0be
Implement Performance.timeOrigin - part 1, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22f17e0db9d
Implement Performance.timeOrigin - part 2 - tests, r=bz

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f869d4c93ef
Implement Performance.timeOrigin - part 3 - tests, r=me

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49305a317143
Implement Performance.timeOrigin - part 4 - tests, r=me

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cf0f72f0b0be
https://hg.mozilla.org/mozilla-central/rev/c22f17e0db9d
https://hg.mozilla.org/mozilla-central/rev/0f869d4c93ef
https://hg.mozilla.org/mozilla-central/rev/49305a317143

Comment 26

[areya]

https://hg.mozilla.org/mozilla-central/rev/cf0f72f0b0be
https://hg.mozilla.org/mozilla-central/rev/c22f17e0db9d
https://hg.mozilla.org/mozilla-central/rev/0f869d4c93ef
https://hg.mozilla.org/mozilla-central/rev/49305a317143

Comment 27

[Chris Mills [:cmills]]

I've documented this by updating the support info on the ref page (it was already documented), and adding a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/docs/Web/API/Performance/timeOrigin#Browser_Compatibility
https://developer.mozilla.org/en-US/Firefox/Releases/59#DOM

Let me know if you think this looks OK. Thanks!

Comment 28

[Chris Mills [:cmills]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> I've documented this by updating the support info on the ref page (it was
> already documented), and adding a note to the Fx59 rel notes:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Performance/
> timeOrigin#Browser_Compatibility
> https://developer.mozilla.org/en-US/Firefox/Releases/59#DOM
> 
> Let me know if you think this looks OK. Thanks!

Ah dammnit, this is a 53 addition. I've moved the note to here:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM