Implement Performance.timeOrigin

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

50 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(2 attachments, 9 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Posted patch timeOrigin.patch (obsolete) — Splinter Review
(Assignee)

Comment 2

3 years ago
I'll add a test in a separate patch.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 4

3 years ago
Posted patch part 1 - nsIXULAppInfo (obsolete) — Splinter Review
Attachment #8805210 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Posted patch part 2 - WebIDL (obsolete) — Splinter Review
(Assignee)

Comment 6

3 years ago
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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 7

3 years ago
Posted patch part 2 - WebIDL (obsolete) — Splinter Review
Attachment #8809801 - Attachment is obsolete: true
> 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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

3 years ago
Posted patch part 1 - timeOrigin (obsolete) — Splinter Review
Attachment #8809800 - Attachment is obsolete: true
Attachment #8809803 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Posted patch part 2 - tests (obsolete) — Splinter Review
(Assignee)

Updated

3 years ago
Flags: needinfo?(bzbarsky)
The part 1 diff is missing the new files....
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 12

3 years ago
Posted patch part 1 - timeOrigin (obsolete) — Splinter Review
Attachment #8810919 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 13

3 years ago
Attachment #8810920 - Attachment is obsolete: true
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".
Flags: needinfo?(bzbarsky)
Attachment #8811166 - Flags: review-
(Assignee)

Comment 15

3 years ago
> 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.
(Assignee)

Comment 16

3 years ago
Posted patch part 1 - timeOrigin (obsolete) — Splinter Review
All the other comments are applied.
Attachment #8811166 - Attachment is obsolete: true
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 18

3 years ago
Posted patch part 1 - timeOrigin (obsolete) — Splinter Review
Ok, I see what you mean. Here a new version.
Attachment #8811455 - Attachment is obsolete: true
Flags: needinfo?(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... :(
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 20

3 years ago
A bit better.
Attachment #8811480 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Comment on attachment 8811511 [details] [diff] [review]
part 1 - timeOrigin

r=me
Flags: needinfo?(bzbarsky)
Attachment #8811511 - Flags: review+

Comment 22

3 years ago
Pushed by amarchesini@mozilla.com:
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

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f869d4c93ef
Implement Performance.timeOrigin - part 3 - tests, r=me

Comment 24

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49305a317143
Implement Performance.timeOrigin - part 4 - tests, r=me
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!
(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
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.