Let stopwatch use window.performance.now when available. Fix for: http://code.google.com/p/dart/iss…

Let stopwatch use window.performance.now when available. Fix for: http://code.google.com/p/dart/issues/detail?id=4312


BUG=4312
Committed: https://code.google.com/p/dart/source/detail?r=15343

[Emily Fortuna, 2012-11-21 01:45:42]

This is the reincarnation of the long abandoned CL:
https://codereview.chromium.org/11232056/

[ngeoffray, 2012-11-21 08:32:53]

https://codereview.chromium.org/11417110/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11417110/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:539: var
performance = JS('var', 'window.performance');
Shouldn't you check that window is defined too?

[kasperl, 2012-11-21 10:27:57]

DBC

https://codereview.chromium.org/11417110/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11417110/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:151: patch static
int _frequency() => 1000;
Wouldn't it be nicer if we returned 1000000 if we can use the high-resolution
timer (window.performance.now) and 1000 if we need to fall back on Date.now()?
That way, we don't need to multiply by 1000 in case we use Date.now() which
seems nicer.

[Emily Fortuna, 2012-11-21 18:49:20]

Good points for both comments. I have addressed both now. PTAL?

[Lasse Reichstein Nielsen, 2012-11-22 11:26:48]

LGTM, but wait for a real dart2js reviewer :)

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:152: patch static
int _now() => Primitives.numMicroseconds();
Side note: We should get rid of these "Primitives" functions and just make them
external directly.
Using "Primitives" is a different way to make a function defined by the backend,
by having the backend introduce the Primitive class/object to the libraries
without exposing it to users. We shouldn't do the same thing in two ways.

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:539: var theWindow
= JS('var', 'window');
This read will throw if 'window' is not there.
Consider using 
 bool windowExists = JS('bool', 'typeof window != "undefined" && window !==
null');
If that is true, then 'window.performance' is safe.

If this is slow, we can consider caching the function, or the fact that the
function exists, so we don't have to check every time. Too much overhead on a
function returning micro seconds seems counter productive :)

[ahe, 2012-11-22 12:54:45]

LGTM when you address Lasse's comment about this:

$ ./xcodebuild/ReleaseIA32/d8
V8 version 3.13.7.5 [console: dumb]
d8> window
(d8):1: ReferenceError: window is not defined
window
^
ReferenceError: window is not defined
    at (d8):1:1

Take a look at this page on how to test dart2js before submitting:
https://code.google.com/p/dart/wiki/TestingDart2js

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:152: patch static
int _now() => Primitives.numMicroseconds();
On 2012/11/22 11:26:48, Lasse Reichstein Nielsen wrote:
> Side note: We should get rid of these "Primitives" functions and just make
them
> external directly.
> Using "Primitives" is a different way to make a function defined by the
backend,
> by having the backend introduce the Primitive class/object to the libraries
> without exposing it to users. We shouldn't do the same thing in two ways.

I disagree, and this is not the forum for discussing the dart2js library design.

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11417110/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:539: var theWindow
= JS('var', 'window');
You don't have to call the variable theWindow.  We know that it is bad to use
the name "window" for a local variable and calls it something else.

[Emily Fortuna, 2012-11-26 20:15:33]

Lasse, Peter,
Uploaded a new version of the patch that addressed your comments.

[ahe, 2012-11-26 20:28:28]

Thank you, Emily! LGTM