Use Zone to correctly measure async operations.

Use Zone to correctly measure async operations.

R=floitsch@google.com, johnniwinther@google.com

Committed: https://github.com/dart-lang/sdk/commit/3c93857ea080749ecf274d864bc96734ae3df43f

[ahe, 2015-11-19 09:52:22]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2015-11-19 10:02:18]

lgtm

https://codereview.chromium.org/1454373002/diff/1/pkg/compiler/lib/src/compil...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1454373002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:2210: measure(action()) => watch == null ?
action() : measureZoned(action);
Add doc

https://codereview.chromium.org/1454373002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:2212: CompilerTask start() {
Add doc

https://codereview.chromium.org/1454373002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:2220: void stop(CompilerTask previous) {
Add doc

https://codereview.chromium.org/1454373002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:2226: measureZoned(action()) {
Add doc.

https://codereview.chromium.org/1454373002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:2262: measureIo(action()) => watch == null ?
action() : measureIoHelper(action);
Add doc

[ahe, 2016-01-06 21:10:24]

ahe@google.com changed reviewers:
+ floitsch@google.com

[ahe, 2016-01-06 21:10:24]

PTAL

[Johnni Winther, 2016-01-07 12:54:46]

Still LGTM but wait for floitsch comments on zones.

[floitsch, 2016-01-07 14:40:40]

LGTM.

Some nits (still have my dart-readability hat on...)

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:61: : super( // Call super first to ensure
Measurer is instantiated first.
Tbh I would prefer if super stays last, and you just had a factory constructor.
It would make it conceptually clearer too:

factory CompilerImpl(...) {
  // If we don't have a measurer yet, instantiate one now before we do
  // any other work.
  measurer ??= new Measurer();
  return new CompilerImpl._(...);
}

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:62: measurer: measurer == null ? new
Measurer() : measurer,
measurer ?? new Measurer()

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:508: Double percent =
unaccountedDuration.inMilliseconds * 100
double is lower case.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/tasks.dart (right):

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:48: bool get isRunning => watch != null
&& watch.isRunning;
fyi: there is a pattern emerging for this: watch?.isRunning == true;

Not saying you should change.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:115: /// various state is set up
correctly (in particular in the root
in particular `Zone.current`

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:131: /// Equivalent to [run] except that
[f] takes one argument, [arg].
nit: s/Equivalent to/Same as/

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:163: /// Note: we assume that this method
is used only by the compiler input
New line before "Note:".

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:177: throw "Can't track async task
'$name' because"
Generally we avoid throwing just strings.
This looks like a StateError to me.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:235: // Constructor must be first to
ensure [wallclock] is started before other
I'm not sure I understand the comment. It reads as if the constructor must be
the first thing in the class, but that has obviously no effect on the semantics.

Same for Constructor and ConstructorImpl.

[ahe, 2016-01-08 08:59:36]

Patchset #5 (id:80001) has been deleted

[ahe, 2016-01-08 09:02:02]

PTAL

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:61: : super( // Call super first to ensure
Measurer is instantiated first.
On 2016/01/07 14:40:39, floitsch wrote:
> Tbh I would prefer if super stays last, and you just had a factory
constructor.
> It would make it conceptually clearer too:
> 
> factory CompilerImpl(...) {
>   // If we don't have a measurer yet, instantiate one now before we do
>   // any other work.
>   measurer ??= new Measurer();
>   return new CompilerImpl._(...);
> }

Code removed, but FWIW, I can't use a factory constructor as Fletch subclasses
this class.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:62: measurer: measurer == null ? new
Measurer() : measurer,
On 2016/01/07 14:40:39, floitsch wrote:
> measurer ?? new Measurer()

Code removed.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:508: Double percent =
unaccountedDuration.inMilliseconds * 100
On 2016/01/07 14:40:39, floitsch wrote:
> double is lower case.

Done.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/tasks.dart (right):

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:48: bool get isRunning => watch != null
&& watch.isRunning;
On 2016/01/07 14:40:39, floitsch wrote:
> fyi: there is a pattern emerging for this: watch?.isRunning == true;
> 
> Not saying you should change.

Done.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:115: /// various state is set up
correctly (in particular in the root
On 2016/01/07 14:40:39, floitsch wrote:
> in particular `Zone.current`

Done.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:131: /// Equivalent to [run] except that
[f] takes one argument, [arg].
On 2016/01/07 14:40:40, floitsch wrote:
> nit: s/Equivalent to/Same as/

Done.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:163: /// Note: we assume that this method
is used only by the compiler input
On 2016/01/07 14:40:39, floitsch wrote:
> New line before "Note:".

Done.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:177: throw "Can't track async task
'$name' because"
On 2016/01/07 14:40:39, floitsch wrote:
> Generally we avoid throwing just strings.

Why?

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:235: // Constructor must be first to
ensure [wallclock] is started before other
On 2016/01/07 14:40:39, floitsch wrote:
> I'm not sure I understand the comment. It reads as if the constructor must be
> the first thing in the class, but that has obviously no effect on the
semantics.
> 
> Same for Constructor and ConstructorImpl.

This is from the Dart Programming Language Specification, 3rd Edition / June
2015, 10.6.1 Generative Constructors:

"Any initializing formals declared in k’s parameter list are executed in the
order they appear in the program text. Then, k’s initializers are executed in
the order they appear in the program."

(k is the constructor).

I misunderstood this to include variable initializers. However, later on, in
16.12.1 New:

"A fresh instance (10.6.1), i, of class R is allocated. For each instance
variable f of i, if the variable declaration of f has an initializer expression
ef , then ef is evaluated, with the type parameters (if any) of R bound to the
actual type arguments V1,...,Vl, to an object of and f is bound to of. Otherwise
f is bound to null.

[...]

Next, q is executed with this bound to i, the type parameters (if any) of R
bound to the actual type arguments V1, . . . , Vl and the formal parameter
bindings that resulted from the evaluation of the argument list. The result of
the evaluation of e is i."

(q is the constructor)

So the correct solution is actually much simpler and was already implemented:
initializing wallClock should be the first field. I'll move the constructors
back. I'm really happy about this :-)

[ahe, 2016-01-08 10:26:40]

Description was changed from

==========
Use Zone to correctly measure async operations.
==========

to

==========
Use Zone to correctly measure async operations.

R=floitsch@google.com, johnniwinther@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3c93857ea080749ecf274d864bc96734ae3df43f
==========

[ahe, 2016-01-08 10:26:42]

Committed patchset #5 (id:100001) manually as
3c93857ea080749ecf274d864bc96734ae3df43f (presubmit successful).

[floitsch, 2016-01-08 12:07:48]

Still LGTM.

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/tasks.dart (right):

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:177: throw "Can't track async task
'$name' because"
On 2016/01/08 09:02:02, ahe wrote:
> On 2016/01/07 14:40:39, floitsch wrote:
> > Generally we avoid throwing just strings.
> 
> Why?

Long thread on dart-readability, and Bob is in the process of updating the style
guide.

If it's an error, then marking it as such, makes the intent clear. Errors also
have some minor advantages (like capturing the stacktrace).

If it's an exception, then you want a class (probably subclass of Exception), so
that the `catch on X` works.

[ahe, 2016-01-08 12:23:45]

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/tasks.dart (right):

https://codereview.chromium.org/1454373002/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/tasks.dart:177: throw "Can't track async task
'$name' because"
On 2016/01/08 12:07:48, floitsch wrote:
> If it's an error, then marking it as such, makes the intent clear.

I disagree. This error message is harder to read:

Internal error: StateError: Can't track async task 'foo' because 'bar' is
already being tracked.

Than this:

Internal error: Can't track async task 'foo' because 'bar' is already being
tracked.

The problem is that "StateError" is irrelevant to the problem, yet people have a
tendency to stop reading after they see "StateError". Same argument could be
made for "Internal error" prefix, but we've decided that users need to know
before anything else that they've encountered a bug in the compiler, and they
didn't do anything wrong. As each prefix adds noise, I think it is important to
keep them to a minimum.

> Errors also have some minor advantages (like capturing the stacktrace).

That's actually a disadvantage: the automatic stack trace capturing in Error
makes debugging harder in JavaScript. Also, capturing stack traces is
unnecessary since we add the second argument to asynchronous error handlers.