Bug 66684 – Implement high-resolution time via window.performance.now()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

Expose monotonic clock to JS via performance.now()

[Nat Duca]

Created attachment 104696 [details]
Patch

[Nat Duca]

Putting up an experimental performance.now() implementation to trigger conversation/discussion. My expectation is that we take our time committing this.

Open to suggestions on what tests would look like for this.

[James Simonsen]

Comment on attachment 104696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104696&action=review

> Source/WebCore/page/Performance.cpp:47
> +    , m_timebase(WTF::monotonicallyIncreasingTime())

FIXME: Use the official document timebase.

[WebKit Review Bot]

Comment on attachment 104696 [details]
Patch

Attachment 104696 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9467867

New failing tests:
fast/dom/Window/window-properties-performance.html

[James Robinson]

FYI I proposed this on WHATWG here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032942.html, but haven't received any response yet.

[Tony Gentilcore]

(In reply to comment #5)
> FYI I proposed this on WHATWG here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032942.html, but haven't received any response yet.

Has this been floated on public-web-perf at all? Maybe it should be discussed as part of the UserTiming spec?

[James Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > FYI I proposed this on WHATWG here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032942.html, but haven't received any response yet.
> 
> Has this been floated on public-web-perf at all? Maybe it should be discussed as part of the UserTiming spec?

I don't think it would be very useful to float this on public-web-perf, personally.

[Tony Gentilcore]

Comment on attachment 104696 [details]
Patch

r- to clear this from the review queue for now.

I like the API, but it is clear this can't land in its current form. At minimum this needs to:
- Have some more substantial path towards standardization than an email to whatwg w/ no responses
- Have a webkit vendor prefix until standardized
- Update the failing fast/dom/Window/window-properties-performance.html test (and preferably add a new test)

[Nat Duca]

(In reply to comment #8)
> - Have some more substantial path towards standardization than an email to whatwg w/ no responses

Wait, I thought we discussed this on a perf wg conf call...

[Tony Gentilcore]

(In reply to comment #9)
> (In reply to comment #8)
> > - Have some more substantial path towards standardization than an email to whatwg w/ no responses
> 
> Wait, I thought we discussed this on a perf wg conf call...

I don't recall it, and didn't find anything searching the list archive (which contains call notes). Although it is very possible I missed it.

James mentioned he doesn't think it is worth floating it in that group. Seems like the logical place to me given that's where window.performance was standardized. But that's not really important. I just want to make sure it is being specced/discussed somewhere.

[James Simonsen]

(In reply to comment #9)
> (In reply to comment #8)
> > - Have some more substantial path towards standardization than an email to whatwg w/ no responses
> 
> Wait, I thought we discussed this on a perf wg conf call...

I thought we'd mentioned it too, but I'm not sure and will defer to the list archive, which says no.

Either way, I'll bring it up next week at TPAC.

[James Robinson]

I've discussed it on WHATWG and on #whatwg as well as , which I think is sufficient standardization discussion for now.  Once we land a vendor-prefixed implementation I plan to continue that discussion in that forum. I think the whatwg is a more useful forum for this particular API than the web perf WG as a number of other APIs are interesting in using this infrastructure.

[Pablo Garaizar]

Do you know when window.performance.now() will be available in a nightly build?

[James Simonsen]

(In reply to comment #13)
> Do you know when window.performance.now() will be available in a nightly build?

I was hoping we'd have a first draft of the spec before we landed the code. I haven't heard back from the WG about when the spec will be ready though. I'll ask during the conference call Wednesday.

[Andy Wingo]

(In reply to comment #0)
> Expose monotonic clock to JS via performance.now()

Dumb question: does this have security implications?  Does it make side channel attacks significantly more effective?  Apologies if this has already been discussed somewhere.

[James Simonsen]

(In reply to comment #15)
> (In reply to comment #0)
> > Expose monotonic clock to JS via performance.now()
> 
> Dumb question: does this have security implications?  Does it make side channel attacks significantly more effective?  Apologies if this has already been discussed somewhere.

Good question. We're going to cover that in the W3C spec. The discussion will likely start on the public-webperf mailing list next week.

[James Simonsen]

Comment on attachment 104696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104696&action=review

Nat, I think I heard you volunteer to update this patch now that the spec is up. I think we can push this vendor prefixed sometime soon. We should probably allow some time for the spec to be discussed first though.

> Source/WebCore/page/Performance.cpp:95
> +    return 1000 * (monotonicallyIncreasingTime() - m_timebase);

This should now be m_frame->document()->loader()->convertMonotonicTimeToDocumentTime(monotonicallyIncreasingTime()). (phew)

> Source/WebCore/page/Performance.idl:41
> +        double now();

The spec returns a typedef.

[Nat Duca]

(In reply to comment #17)
> Nat, I think I heard you volunteer to update this patch now that the spec is up. I think we can push this vendor prefixed sometime soon. We should probably allow some time for the spec to be discussed first though.

Yep! Exited about getting this exposed. :) Would be so nice to report 60fps for a change.

Ill wait a week or two until it looks like webperf is getting happyish with it then I'll post a fixed up patch.

[Tony Gentilcore]

> Ill wait a week or two until it looks like webperf is getting happyish with it then I'll post a fixed up patch.

The working draft specification has been stable since Mar 13th:
http://www.w3.org/TR/hr-time/

[Nat Duca]

(In reply to comment #19)
> > Ill wait a week or two until it looks like webperf is getting happyish with it then I'll post a fixed up patch.

Thanks for reminding me. I put it on my short list. Things have been a little crazy.

[Nat Duca]

Created attachment 138256 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138256 [details]
Patch

Attachment 138256 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12474573

New failing tests:
fast/dom/Window/window-properties-performance.html

[WebKit Review Bot]

Created attachment 138258 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Gentilcore]

Comment on attachment 138256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138256&action=review

Thanks for the updates. Looking good, just a few minor comments to address.

> Source/WebCore/page/Performance.cpp:75
> +    return 1000.0 * m_frame->document()->loader()->timing()->convertMonotonicTimeToDocumentTime(monotonicallyIncreasingTime());

The spec says to return milliseconds, but if I'm reading this right, you are converting to microseconds.

Since Date.now() and everything in performance.timing use ms, we should probably stick to that, right? This is a double, so the microsecond precision will be maintained.

> Source/WebCore/page/Performance.idl:43
> +        double now();

According to the spec, this should be:
typedef double DOMHighResTimeStamp;
DOMHighResTimeStamp now();

That's analogous to the DOMTimeStamp used by other APIs.

Also, I've lost track, what is our current stance on vendor prefixing? Should this be webkitNow() until the spec reaches CR?

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:12
> +busyWait(10);

Because the condition below uses ">=", this wait doesn't add any value to the test.

You could either just remove the wait or else change the test below to something like "secondTimestamp > (firstTimestamp + (WAIT_TIME / 2))"

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:16
> +shouldBeTrue("secondTimestamp >= firstTimestamp");

Any reason to use shouldBeGreaterThanOrEqual above but ">=" directly here?

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:17
> +

Can you add a reasonable but generous <= condition to verify this is in the correct range (ie. ms since navigationStart as opposed to ms since epoch or microseconds since anything)?

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:19
> +    layoutTestController.waitUntilDone();

There is no reason for this test to waitUntilDone. Please make it synchronous. That will also fix the double TEST_COMPLETE message in your expectations.

[James Robinson]

(In reply to comment #24)
> (From update of attachment 138256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138256&action=review
> 
> Thanks for the updates. Looking good, just a few minor comments to address.
> 
> > Source/WebCore/page/Performance.cpp:75
> > +    return 1000.0 * m_frame->document()->loader()->timing()->convertMonotonicTimeToDocumentTime(monotonicallyIncreasingTime());
> 
> The spec says to return milliseconds, but if I'm reading this right, you are converting to microseconds.
> 
> Since Date.now() and everything in performance.timing use ms, we should probably stick to that, right? This is a double, so the microsecond precision will be maintained.

Timesources in wtf/CurrentTime.h like WTF::currentTime() and WTF::monotonicallyIncreasingTime() are all in seconds, so conversions to DOMTimeStamp have a * 1000.0.  I would expect convertMonotonicTimeToDocumentTime to also use the seconds-in-doubles convention, does it not?

> 
> > Source/WebCore/page/Performance.idl:43
> > +        double now();
> 
> According to the spec, this should be:
> typedef double DOMHighResTimeStamp;
> DOMHighResTimeStamp now();
> 
> That's analogous to the DOMTimeStamp used by other APIs.

DOMHighResTimeStamp is a spec-only concept, it shouldn't be exposed to the web or IDL.  We should just keep it double in the IDL - having a different type for the bindings generators doesn't help anything.

[Tony Gentilcore]

> Timesources in wtf/CurrentTime.h like WTF::currentTime() and WTF::monotonicallyIncreasingTime() are all in seconds, so conversions to DOMTimeStamp have a * 1000.0.  I would expect convertMonotonicTimeToDocumentTime to also use the seconds-in-doubles convention, does it not?

Not sure by immediate inspection. Anyway, I suggested a test to make sure the value is in the right range which should clear up any doubt.

> We should just keep it double in the IDL

SGTM - thanks for clarifying

[Nat Duca]

Patch upcoming. Confirmed that the *1000 is required, and added tests to verify, great idea Tony.

[Nat Duca]

Created attachment 138745 [details]
Patch

[Tony Gentilcore]

Comment on attachment 138745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138745&action=review

A few last things. Everything else looks good to me.

> LayoutTests/fast/dom/Window/window-properties-performance-expected.txt:15
> +window.performance.navigation.now [function]

webkitNow

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:10
> +shouldBeGreaterThanOrEqual("firstTimestamp", 0);

> Can you add a reasonable but generous <= condition to verify this is in the correct range (ie. ms since navigationStart as opposed to ms since epoch or microseconds since anything)?

What I had in mind in that comment was to add a test like:

// Verify returned value is in milliseconds since navigationStart.
// This generously assumes this JS will run within the first 5 seconds while ruling out the possibility that the returned value is in milliseconds since epoch.
shouldBeTrue("firstTimestamp < 5000");

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:17
> +shouldBeTrue("secondTimestamp >= firstTimestamp + (waitTime / 2)");

Still wondering why shouldBeGreaterThanOrEqual is used above but shouldBeTrue with a >= is used here. Should be consistent.

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:22
> + */

I believe WebKit prefers single line comments.

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:25
> +shouldBeTrue("difference > 1 && difference < 100");

Breaking this out into two shouldBeTrues will provide a better failure message when it breaks.

> LayoutTests/platform/qt/fast/dom/Window/window-properties-performance-expected.txt:15
> +window.performance.navigation.now [function]

webkitNow

[WebKit Review Bot]

Comment on attachment 138745 [details]
Patch

Attachment 138745 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12527581

New failing tests:
fast/dom/Window/window-properties-performance.html

[WebKit Review Bot]

Created attachment 138805 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nat Duca]

Created attachment 138901 [details]
Patch

[Nat Duca]

> > Can you add a reasonable but generous <= condition to verify this is in the correct range (ie. ms since navigationStart as opposed to ms since epoch or microseconds since anything)?
> 
> What I had in mind in that comment was to add a test like:
> 
> // Verify returned value is in milliseconds since navigationStart.
> // This generously assumes this JS will run within the first 5 seconds while ruling out the possibility that the returned value is in milliseconds since epoch.
> shouldBeTrue("firstTimestamp < 5000");

So, we don't currently have zeroed timebases yet, as the original spec didn't have this. Talked briefly to @simonjam, he plans to file a bug to track moving the timebases to zero. When he does that, this code will automatically benefit.

Whether we want to do this first, or the zeroing patch is something I'm open to feedback on.

[Tony Gentilcore]

(In reply to comment #33)
> > > Can you add a reasonable but generous <= condition to verify this is in the correct range (ie. ms since navigationStart as opposed to ms since epoch or microseconds since anything)?
> > 
> > What I had in mind in that comment was to add a test like:
> > 
> > // Verify returned value is in milliseconds since navigationStart.
> > // This generously assumes this JS will run within the first 5 seconds while ruling out the possibility that the returned value is in milliseconds since epoch.
> > shouldBeTrue("firstTimestamp < 5000");
> 
> So, we don't currently have zeroed timebases yet, as the original spec didn't have this. Talked briefly to @simonjam, he plans to file a bug to track moving the timebases to zero. When he does that, this code will automatically benefit.
> 
> Whether we want to do this first, or the zeroing patch is something I'm open to feedback on.

I think we should do that before landing this. It doesn't seem ideal to expose a behavior that we are about to change. That being said, that subtraction seems really trivial to in this patch. If you like you just subtract it out in Performance::webkitNow(), and leave the refactor to bug 84912, right?

[Nat Duca]

@simonjam, what's your sense, based on Tony's feedback?

[James Simonsen]

(In reply to comment #35)
> @simonjam, what's your sense, based on Tony's feedback?

SGTM. I like the temporary fix of subtracting it in webkitNow() for the time being. Perhaps include a FIXME to remove it when the other bug is resolved.

[Nat Duca]

Created attachment 139155 [details]
ZeroBasedTime

[Nat Duca]

(In reply to comment #36)
> SGTM. I like the temporary fix of subtracting it in webkitNow() for the time being. Perhaps include a FIXME to remove it when the other bug is resolved.

I had to make a new method on DocumentLoadTiming for convertBlahBlahToZeroBasedTime because the actual timebase we needed to subtract is sealed inside the timing class. This method duplication is actually kind of nice, I think, on two fronts:
1) it keeps the timebase sealed inside the timing class, still
2) b66683 also needs this zero-based time --- this keeps the "quick workaround" in one place rather than in both.

[Tony Gentilcore]

Comment on attachment 139155 [details]
ZeroBasedTime

View in context: https://bugs.webkit.org/attachment.cgi?id=139155&action=review

Thanks again for the updates. Sending an r+ now, but it is probably worth double checking w/ simonjam to make sure he is happy before landing.

> Source/WebCore/ChangeLog:3
> +        Implement high-resolution time via window.performance.now()

Probably worth mentioning the prefixing in the ChangeLog somewhere.

> Source/WebCore/loader/DocumentLoadTiming.h:48
> +    double convertMonotonicTimeToZeroBasedDocumentTime(double monotonicTime) const;

Nit: the param really doesn't need to be named here or above. It is redundant with the method name.

[James Simonsen]

(In reply to comment #39)
> (From update of attachment 139155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139155&action=review
> 
> Thanks again for the updates. Sending an r+ now, but it is probably worth double checking w/ simonjam to make sure he is happy before landing.

LGTM

[Nat Duca]

Created attachment 139231 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139231 [details]
Patch for landing

Clearing flags on attachment: 139231

Committed r115503: <http://trac.webkit.org/changeset/115503>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.