Bug 66684

Created attachment 104696 [details] Patch

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

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.

(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?

(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 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)

(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...

(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.

(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.

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.

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

(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.

(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.

(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 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.

(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.

> 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/

(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.

Created attachment 138256 [details] Patch

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

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 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.

(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.

> 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

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

Created attachment 138745 [details] Patch

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

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

Created attachment 138901 [details] Patch

> > 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.

(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?

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

(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.

Created attachment 139155 [details] ZeroBasedTime

(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 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.

(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

Created attachment 139231 [details] Patch for landing

Comment on attachment 139231 [details] Patch for landing Clearing flags on attachment: 139231 Committed r115503: <http://trac.webkit.org/changeset/115503>

All reviewed patches have been landed. Closing bug.