1169068 - Update the High Resolution Time API to the latest version of the spec

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

The draft of the spec introduces workerStart and performance.now() in workers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should get the spec figured out.  I strongly believe the zero time should have a unified name across windows and workers.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: time.patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8612157 [details] [diff] [review]
time.patch

It's not clear to me that this matches the spec (e.g. it always returns 0 for a shared worker, no?).

That said, the spec seems entirely unclear to me; I'll post to the list about that.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: time.patch

Thanks for spotting this, bz.
Would be nice to have in the spec that, for SharedWorkers/ServiceWorkers workerStart can be lower than navigationStart.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: time.patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

No, that doesn't make sense either, because that's subtracting wall-clock values not TimeStamp values.

What you want to be doing is taking the difference between the CreationTimeStamp() of the WorkerPrivate and the GetNavigationStartTimeStamp() of the navigation timing or so.  But do note the mail I just sent to the spec list a bit ago...

Comment 7

[(no longer active)]

Comment on attachment 8612258 [details] [diff] [review]
time.patch

Clearing per comment 6.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: time.patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8622520 [details] [diff] [review]
time.patch

The workerStart thing isn't pinned down in the spec and unlikely to stay in the form it's in right now.  See the public-web-perf discussion.

We should probably wrap up said discussion before implementing this.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: time.patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689097 [details] [diff] [review]
time.patch

>-  // If the time we're trying to convert is equal to zero, it hasn't been set
>-  // yet so just return 0.

Hmm.  So I guess the only caller of this was PerformanceBase::ResolveTimestampFromName, which never calls this function with 0, right?  <sigh>.

>+nsPerformance::CreationTimeStamp() const

I think this should return an actual TimeStamp or "const Timestamp&" or something, not a DOMHighResTimeStamp.  Because otherwise you're using non-monotonic clocks for the values you're working with, which doesn't seem correct.  I filed https://github.com/w3c/hr-time/issues/21 on the spec not being sufficiently clear here.

So you'd return GetDOMTiming()->GetNavigationStartTimeStamp() from this method.

>+PerformanceBase::TranslateTime(DOMHighResTimeStamp aTime,

And here what you'd want to do is to compute the otherCreationTimeStamp across various codepaths (window, various workers) and then do:

  double newTime = 
    aTime + (otherCreationTimeStamp - CreationTimeStamp()).toMilliseconds();
  const double maxResolutionMs = 0.005;
  return floor(newTime / maxResolutionMs) * maxResolutionMs;

It may be worth factoring this clampingto 5us out into an inline method on PerformanceBase, now that we'll have it in three different places...

>+  return aTime + (workerPrivate->NowBaseTimeHighRes() - CreationTimeStamp());

And instead of NowBaseTimeHighRes() you'd just call NowBaseTimeStamp() here to get the otherCreationTimeStamp.

>+  return ts - CreationTimeStamp();

I guess you'd want a CreationTime() method too, which does what CreationTimeStamp() does in your code above, for use here.

>+++ b/dom/base/test/test_performance_translate.html
>+    ok (a > 0, "Time has been translate from a window");

I'm afraid the best you can assert here is a >= 0, especially once the 5us clamping is introduced.

Also, I think this test would be stronger if you compared 0 translated into the subframe to performance.now() captured right before the start of the subframe load.

Also, "has been translated from a window that started loading later than we did".

>+  ok (a > 0, "Time has been translate from a Worker");

Similar comments here.

>+  ok (a > 0, "Time has been translate from a SharedWorker");

And here.

>+++ b/dom/webidl/Performance.webidl
>+  // serializer = {attribute};

We already implement this: it's the "jsonifier" in the partial interface block immediately following.  Just move it up here?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and I guess file a followup on changing what the actual time base of a dedicated worker is, right?

Comment 13

[Andrea Marchesini [:baku]]

Attachment: time.patch

I cannot move jsonifier in the previous block otherwise webIDL gencode will complain about get_timing and get_navigation not implemented in workers.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I cannot move jsonifier in the previous block

Oh, I see.  The jsonifier is mainthread-only right now.

I guess that's OK, since the attributes it would affect are also mainthread-only.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689279 [details] [diff] [review]
time.patch

>+    RefPtr<nsPerformance> performance = aTimeSource.GetAsWindow().GetPerformance();
>+    MOZ_ASSERT(performance);

GetPerformance() can, unfortunately, return null.  Sorry I missed that the first time.

I think it's reasonable to just return aTime in that situation or something; it's not something that should come up in practice in non-malicious usage.  Or we could throw.  Either way.

>+    otherCreationTimeStamp = aTimeSource.GetAsWorker().CreationTimeStamp();
>+    otherCreationTimeStamp = workerPrivate->CreationTimeStamp();
>+    otherCreationTimeStamp = workerPrivate->CreationTimeStamp();

Thse should all be NowBaseTimeStamp(), no?  At least until we get rid of the difference in terms of how Performance.now() and whatnot work on workers.

I wonder how we could add a test that would have caught this....  I guess long-term, when CreationTimeStamp becomes NowBaseTimeStamp(), we can't.

>+PerformanceBase::AdjustTime(double aTime) const

I'd call this RoundTime().

>+++ b/dom/base/test/test_performance_translate.html
>+  ok (a >= 0, "Time has been translated from a Worker that started loading later than we did");

Again, would be better to compare it to a performance.now() call from right before the new Worker() call.

Except I guess that wouldn't work for now while the worker shares our timeline, right?  Maybe add a comment and a todo() to that effect for now.

>+  ok (a >= 0, "Time has been translated from a SharedWorker that started loading later than we did");

This one, though, should be comparable to performance.now() from right before new SharedWorker, right?

r=me with the above fixed.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c466e1e3d71

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c466e1e3d71

Comment 18

[Boris Zbarsky [:bzbarsky]]

I just realized that we really should have sent an intent to ship.  Done that now.

Comment 19

[Kohei Yoshino [:kohei]]

Backed out in Bug 1243881.