Introduce and use Zone:_enter and Zone:_leave, in Future.

Introduce and use Zone:_enter and Zone:_leave, in Future.

This is the first step toward clearer zone specification usage, and
potential ZoneSpecification.onEnter/onLeave callbacks.

BUG=
R=floitsch@google.com, lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32123

[Lasse Reichstein Nielsen, 2014-01-28 12:40:19]

LGTM, but don't commit until performance is acceptable.

Consider tests: Is there some way to test this, and attempt to break it by
having futures from different zones depending on each other in odd ways?

[floitsch, 2014-01-28 13:18:14]

DBC.

https://codereview.chromium.org/136113014/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/136113014/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:395: var previous = _current;
Don't use "var".

[Anders Johnsen, 2014-01-28 13:43:34]

PTAL, large change to speed it up.

https://codereview.chromium.org/136113014/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/136113014/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:395: var previous = _current;
On 2014/01/28 13:18:15, floitsch wrote:
> Don't use "var".

Done.

[floitsch, 2014-01-28 14:22:45]

FYI.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:438: bool handleValue() {
add new line before "handleValue".

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:455: bool handleError() {
new line before nested function.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:471: // TODO(ajohnsen): Use either runUnary or
runBinary.
if (errorCallback is ZoneBinaryCallback) {
  listenerValueOrError = zone.runBinary(errorCallback, asyncError.error,
asyncError.stackTrace);
} else {
  listenerValueOrError = zone.runUnary(errorCallback, asyncError.error);
}

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:491: void handleWhenComplete() {
New line before nested function.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:512: } catch (e, s) {
The try/catch can just surround the whenCompleteAction callback.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:522: if (!hasError) {
I think there is no case where there is a value/error handler and at the same
time a complete-handler.
This can thus be simplified to:

if (hasError) {
  handleError();
} else {
  handleValue();
}
handleWhenComplete();

[Anders Johnsen, 2014-01-29 08:00:28]

PTAL. Also, Futures are now ~10% faster than without this CL.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:438: bool handleValue() {
On 2014/01/28 14:22:45, floitsch wrote:
> add new line before "handleValue".

Done.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:455: bool handleError() {
On 2014/01/28 14:22:45, floitsch wrote:
> new line before nested function.

Done.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:471: // TODO(ajohnsen): Use either runUnary or
runBinary.
On 2014/01/28 14:22:45, floitsch wrote:
> if (errorCallback is ZoneBinaryCallback) {
>   listenerValueOrError = zone.runBinary(errorCallback, asyncError.error,
> asyncError.stackTrace);
> } else {
>   listenerValueOrError = zone.runUnary(errorCallback, asyncError.error);
> }

Done.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:491: void handleWhenComplete() {
On 2014/01/28 14:22:45, floitsch wrote:
> New line before nested function.

Done.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:512: } catch (e, s) {
On 2014/01/28 14:22:45, floitsch wrote:
> The try/catch can just surround the whenCompleteAction callback.

Done.

https://codereview.chromium.org/136113014/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:522: if (!hasError) {
On 2014/01/28 14:22:45, floitsch wrote:
> I think there is no case where there is a value/error handler and at the same
> time a complete-handler.
> This can thus be simplified to:
> 
> if (hasError) {
>   handleError();
> } else {
>   handleValue();
> }
> handleWhenComplete();

Sadly, depending on if any error was cough, handleWhenComplete should not be
called.

[floitsch, 2014-01-29 13:35:45]

LGTM.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:422: Zone old;
Nit: I would prefer "oldZone", but "old" doesn't seem to conflict. so ok with
me.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:433: // At this point we are in the right zone.
Each callback is invoked
You can just remove those two lines. I'm not sure they add anything.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:458: listenerValueOrError =
identical(asyncError.error, e) ?
I wonder if we shouldn't change the semantics here.
If the test throws, it should probably be considered as a real throw. Only the
catch-Error function should be able to "rethrow".

Maybe ask Lasse what he thinks.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:505: // Try again, but this time don't run the
whenComplete callback.
// Try again. Since the future is marked as chained it won't run the
whenComplete again.

?

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:517: isPropagationAborted = true;
move to before the completeResult.then?

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:528: if (listenerHasValue &&
listener._whenCompleteAction != null) {
I don't think you need this.
The _whenCompleteAction and _onValue action can not happen at the same time.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:529: handleWhenCompleteCallback();
It would be easier to optimize, if handleWhenCompleteCallback was only invoked
once. Then the compiler (or VM) could see that there is only one use, and just
inline it there (without code-bloat).

[Anders Johnsen, 2014-01-29 15:30:33]

Thanks, cleaner up some stuff :)

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:422: Zone old;
On 2014/01/29 13:35:46, floitsch wrote:
> Nit: I would prefer "oldZone", but "old" doesn't seem to conflict. so ok with
> me.

Done.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:433: // At this point we are in the right zone.
Each callback is invoked
On 2014/01/29 13:35:46, floitsch wrote:
> You can just remove those two lines. I'm not sure they add anything.

Done.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:458: listenerValueOrError =
identical(asyncError.error, e) ?
On 2014/01/29 13:35:46, floitsch wrote:
> I wonder if we shouldn't change the semantics here.
> If the test throws, it should probably be considered as a real throw. Only the
> catch-Error function should be able to "rethrow".
> 
> Maybe ask Lasse what he thinks.

Adding TODO to be sure we revisit this.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:505: // Try again, but this time don't run the
whenComplete callback.
On 2014/01/29 13:35:46, floitsch wrote:
> // Try again. Since the future is marked as chained it won't run the
> whenComplete again.
> 
> ?

Done.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:517: isPropagationAborted = true;
On 2014/01/29 13:35:46, floitsch wrote:
> move to before the completeResult.then?

Done.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:528: if (listenerHasValue &&
listener._whenCompleteAction != null) {
On 2014/01/29 13:35:46, floitsch wrote:
> I don't think you need this.
> The _whenCompleteAction and _onValue action can not happen at the same time.

Done.

https://codereview.chromium.org/136113014/diff/60001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:529: handleWhenCompleteCallback();
On 2014/01/29 13:35:46, floitsch wrote:
> It would be easier to optimize, if handleWhenCompleteCallback was only invoked
> once. Then the compiler (or VM) could see that there is only one use, and just
> inline it there (without code-bloat).

Done.

[Anders Johnsen, 2014-01-29 15:31:20]

Committed patchset #5 manually as r32123 (presubmit successful).