Implement Performance.timeOrigin

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
RESOLVED FIXED
a year ago
7 days ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-needed})

50 Branch
mozilla53
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(2 attachments, 9 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8805210 [details] [diff] [review]
timeOrigin.patch
(Assignee)

Comment 2

a year 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

a year ago
Created attachment 8809800 [details] [diff] [review]
part 1 - nsIXULAppInfo
Attachment #8805210 - Attachment is obsolete: true
(Assignee)

Comment 5

a year ago
Created attachment 8809801 [details] [diff] [review]
part 2 - WebIDL
(Assignee)

Comment 6

a year 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

a year ago
Created attachment 8809803 [details] [diff] [review]
part 2 - WebIDL
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

a year ago
Created attachment 8810919 [details] [diff] [review]
part 1 - timeOrigin
Attachment #8809800 - Attachment is obsolete: true
Attachment #8809803 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
Created attachment 8810920 [details] [diff] [review]
part 2 - tests
(Assignee)

Updated

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

Comment 12

a year ago
Created attachment 8811166 [details] [diff] [review]
part 1 - timeOrigin
Attachment #8810919 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 13

a year ago
Created attachment 8811223 [details] [diff] [review]
part 2 - tests
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

a year 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

a year ago
Created attachment 8811455 [details] [diff] [review]
part 1 - timeOrigin

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

a year ago
Created attachment 8811480 [details] [diff] [review]
part 1 - timeOrigin

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

a year ago
Created attachment 8811511 [details] [diff] [review]
part 1 - timeOrigin

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

a year 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

a year 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

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49305a317143
Implement Performance.timeOrigin - part 4 - tests, r=me

Comment 25

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.