Bug 66684 - Implement high-resolution time via window.performance.now()
Summary: Implement high-resolution time via window.performance.now()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL: http://www.w3.org/TR/hr-time/
Keywords: Performance
Depends on: 58354 84912
Blocks: 84386 66683
  Show dependency treegraph
 
Reported: 2011-08-22 10:54 PDT by Nat Duca
Modified: 2012-04-27 15:34 PDT (History)
12 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2011-08-22 10:54 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2012-04-21 15:08 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.04 MB, application/zip)
2012-04-21 16:11 PDT, WebKit Review Bot
no flags Details
Patch (9.20 KB, patch)
2012-04-24 23:57 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (691.79 KB, application/zip)
2012-04-25 07:21 PDT, WebKit Review Bot
no flags Details
Patch (9.18 KB, patch)
2012-04-25 17:04 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
ZeroBasedTime (11.44 KB, patch)
2012-04-27 02:32 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch for landing (11.49 KB, patch)
2012-04-27 11:01 PDT, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-08-22 10:54:09 PDT
Expose monotonic clock to JS via performance.now()
Comment 1 Nat Duca 2011-08-22 10:54:30 PDT
Created attachment 104696 [details]
Patch
Comment 2 Nat Duca 2011-08-22 10:56:13 PDT
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.
Comment 3 James Simonsen 2011-08-22 11:42:57 PDT
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.
Comment 4 WebKit Review Bot 2011-08-22 12:26:37 PDT
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
Comment 5 James Robinson 2011-08-22 12:31:48 PDT
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.
Comment 6 Tony Gentilcore 2011-10-26 10:56:55 PDT
(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?
Comment 7 James Robinson 2011-10-26 11:58:20 PDT
(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.
Comment 8 Tony Gentilcore 2011-10-27 02:09:24 PDT
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)
Comment 9 Nat Duca 2011-10-27 09:48:03 PDT
(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...
Comment 10 Tony Gentilcore 2011-10-27 09:55:00 PDT
(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.
Comment 11 James Simonsen 2011-10-27 11:07:44 PDT
(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.
Comment 12 James Robinson 2011-10-27 11:12:30 PDT
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.
Comment 13 Pablo Garaizar 2011-12-12 01:32:11 PST
Do you know when window.performance.now() will be available in a nightly build?
Comment 14 James Simonsen 2011-12-12 11:19:41 PST
(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.
Comment 15 Andy Wingo 2012-02-10 04:11:19 PST
(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.
Comment 16 James Simonsen 2012-02-10 12:23:57 PST
(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.
Comment 17 James Simonsen 2012-02-28 17:23:42 PST
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.
Comment 18 Nat Duca 2012-02-28 17:36:31 PST
(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.
Comment 19 Tony Gentilcore 2012-04-20 04:02:01 PDT
> 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/
Comment 20 Nat Duca 2012-04-20 04:04:06 PDT
(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.
Comment 21 Nat Duca 2012-04-21 15:08:26 PDT
Created attachment 138256 [details]
Patch
Comment 22 WebKit Review Bot 2012-04-21 16:11:06 PDT
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
Comment 23 WebKit Review Bot 2012-04-21 16:11:12 PDT
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
Comment 24 Tony Gentilcore 2012-04-23 03:10:35 PDT
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.
Comment 25 James Robinson 2012-04-23 11:02:51 PDT
(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.
Comment 26 Tony Gentilcore 2012-04-24 09:58:46 PDT
> 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
Comment 27 Nat Duca 2012-04-24 23:53:07 PDT
Patch upcoming. Confirmed that the *1000 is required, and added tests to verify, great idea Tony.
Comment 28 Nat Duca 2012-04-24 23:57:23 PDT
Created attachment 138745 [details]
Patch
Comment 29 Tony Gentilcore 2012-04-25 05:01:00 PDT
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
Comment 30 WebKit Review Bot 2012-04-25 07:21:02 PDT
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
Comment 31 WebKit Review Bot 2012-04-25 07:21:14 PDT
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
Comment 32 Nat Duca 2012-04-25 17:04:01 PDT
Created attachment 138901 [details]
Patch
Comment 33 Nat Duca 2012-04-25 17:08:03 PDT
> > 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.
Comment 34 Tony Gentilcore 2012-04-26 01:54:57 PDT
(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?
Comment 35 Nat Duca 2012-04-26 12:00:45 PDT
@simonjam, what's your sense, based on Tony's feedback?
Comment 36 James Simonsen 2012-04-26 12:12:17 PDT
(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.
Comment 37 Nat Duca 2012-04-27 02:32:37 PDT
Created attachment 139155 [details]
ZeroBasedTime
Comment 38 Nat Duca 2012-04-27 02:35:35 PDT
(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.
Comment 39 Tony Gentilcore 2012-04-27 03:00:21 PDT
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.
Comment 40 James Simonsen 2012-04-27 10:23:26 PDT
(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
Comment 41 Nat Duca 2012-04-27 11:01:22 PDT
Created attachment 139231 [details]
Patch for landing
Comment 42 WebKit Review Bot 2012-04-27 15:34:04 PDT
Comment on attachment 139231 [details]
Patch for landing

Clearing flags on attachment: 139231

Committed r115503: <http://trac.webkit.org/changeset/115503>
Comment 43 WebKit Review Bot 2012-04-27 15:34:20 PDT
All reviewed patches have been landed.  Closing bug.