Rewrite Futures.

Rewrite Futures.

R=lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27375
Reverted: https://code.google.com/p/dart/source/detail?r=27376

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

[kevmoo-old, 2013-09-09 19:27:44]

DBR silly nit

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:338: _asyncCompleteError(error, [StackTrace
stackTrace]) {
void?

[Lasse Reichstein Nielsen, 2013-09-10 12:03:53]

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:103: Completer completer = new Completer.sync();
WHy not "new _Future()" followed by ".complete" or ".completeError"?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:118: * if the result of executing [computation]
throws, the returned future is
Capital "I".
And reword to "If calling [computation] throws,".

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:176: result = result.then((ignored) => computation());
This throws away the type parameter.
Can we avoid using this strategy?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:114: _Future _nextListener;
Extra field, no documentation? What is this?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:123: final _FutureAction
_whenCompleteActionCallback;
Four more fields? Even two?
How about just one field, and a container for the case when there is more? Or
just have subclasses for the four cases instead?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:132: : _onValueCallback = null,
_errorTestCallback = null,
Default is null, you don't have to write all these.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:225: _resultOrListeners = listener;
Ah, now I get it (where "now" is reading some code below).
The futures that are listeners of a future form a single linked list, and the
_nextListener is the link.

Far too clever abbreviation:
 _resultOrListeners = listener.._nextListener = _resultOrListeners;

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:302: _propagateToSuccessors(this, listeners);
Does it make sense to call this if listeners is null?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:340: _onErrorCallback == null &&
indent to after '('.
Or, preferably, make this four asserts. That way it's easier to see what is
wrong if one of them fails.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:380: _propagateToSuccessors(source, listener);
This seems like an awkward way to do a loop.
Drop the self-recursion and have a helper function that does the actual work,
and a loop calling that for each listener.
Feel free to call the helper function directly, or through a different function
if you want the asserts outside the loop, if you know you have only a single
listener.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:394: _propagateToSuccessors(source, listeners);
listeners -> listener 
(same thing, easier to read).

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:398: // Normal listener.
What does "normal" mean? 
Seems to be "in the same zone as the current"?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:456: completeResult = new _Future();
Is there an .immediateError constructor?

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

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:346: * Executes the [_onDone] callback when the zone is
done.
Return type is "dynamic"?

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:352: _dispose() {
return type should be void.

https://codereview.chromium.org/23926011/diff/10001/tests/lib/async/future_te...
File tests/lib/async/future_test.dart (left):

https://codereview.chromium.org/23926011/diff/10001/tests/lib/async/future_te...
tests/lib/async/future_test.dart:635: 
Why remove the test? Is it not correct any more?

[Lasse Reichstein Nielsen, 2013-09-10 13:52:11]

LGTM if you make the calling-successors loop readable.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:114: _Future _nextListener;
If a single listener is the common case, how about having the list externally,
so _resultOrListeners contains either a single listener, or a (dart2js-like
Link<_Future> structure).
Might need a status bit to tell the difference.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:123: final _FutureAction
_whenCompleteActionCallback;
I am concerned about having the extra overhead on all futures.
I think we can do better, but this is ok as a first implementation.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:132: : _onValueCallback = null,
_errorTestCallback = null,
My bad.

[floitsch, 2013-09-10 17:19:13]

I would like to talk to you about improving the propagation loop. Otherwise most
comments should be addressed (and even if it just meant adding TODOs).

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:103: Completer completer = new Completer.sync();
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> WHy not "new _Future()" followed by ".complete" or ".completeError"?

Done.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:118: * if the result of executing [computation]
throws, the returned future is
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Capital "I".
> And reword to "If calling [computation] throws,".

Done.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:176: result = result.then((ignored) => computation());
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> This throws away the type parameter.
> Can we avoid using this strategy?

Not now.
We will go through the whole library and decide if we should worry or not.
We are not going to start caring randomly.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:114: _Future _nextListener;
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Extra field, no documentation? What is this?

The same as before (line 81). Will add documentation.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:114: _Future _nextListener;
On 2013/09/10 13:52:12, Lasse Reichstein Nielsen wrote:
> If a single listener is the common case, how about having the list externally,
> so _resultOrListeners contains either a single listener, or a (dart2js-like
> Link<_Future> structure).
> Might need a status bit to tell the difference.

Completely agree. Adding a TODO.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:123: final _FutureAction
_whenCompleteActionCallback;
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Four more fields? Even two?
> How about just one field, and a container for the case when there is more? Or
> just have subclasses for the four cases instead?

See TODO above.
Will optimize later.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:123: final _FutureAction
_whenCompleteActionCallback;
On 2013/09/10 13:52:12, Lasse Reichstein Nielsen wrote:
> I am concerned about having the extra overhead on all futures.
> I think we can do better, but this is ok as a first implementation.

Agreed. We will have lots of time to optimize...

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:132: : _onValueCallback = null,
_errorTestCallback = null,
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Default is null, you don't have to write all these.

That is not correct. Final fields need to be initialized. Otherwise there is a
warning.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:225: _resultOrListeners = listener;
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Ah, now I get it (where "now" is reading some code below).
> The futures that are listeners of a future form a single linked list, and the
> _nextListener is the link.
> 
> Far too clever abbreviation:
>  _resultOrListeners = listener.._nextListener = _resultOrListeners;

too clever :)

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:302: _propagateToSuccessors(this, listeners);
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Does it make sense to call this if listeners is null?

Yes. Because that's where we need to trigger an uncaught exception.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:338: _asyncCompleteError(error, [StackTrace
stackTrace]) {
On 2013/09/09 19:27:45, kevmoo wrote:
> void?

Done.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:340: _onErrorCallback == null &&
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> indent to after '('.
> Or, preferably, make this four asserts. That way it's easier to see what is
> wrong if one of them fails.

Done.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:394: _propagateToSuccessors(source, listeners);
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> listeners -> listener 
> (same thing, easier to read).

Done.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:398: // Normal listener.
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> What does "normal" mean? 
> Seems to be "in the same zone as the current"?

Replaced with "// Do actual propagation".

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:456: completeResult = new _Future();
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Is there an .immediateError constructor?

There is, but even though it is called "immediateError" it is completed
asynchronously.

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

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:346: * Executes the [_onDone] callback when the zone is
done.
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Return type is "dynamic"?

Done.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:352: _dispose() {
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> return type should be void.

Done.

https://codereview.chromium.org/23926011/diff/10001/tests/lib/async/future_te...
File tests/lib/async/future_test.dart (left):

https://codereview.chromium.org/23926011/diff/10001/tests/lib/async/future_te...
tests/lib/async/future_test.dart:635: 
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> Why remove the test? Is it not correct any more?

We don't catch cyclic chained futures anymore.

[floitsch, 2013-09-11 09:29:29]

PTAL.

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/23926011/diff/10001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:380: _propagateToSuccessors(source, listener);
On 2013/09/10 12:03:53, Lasse Reichstein Nielsen wrote:
> This seems like an awkward way to do a loop.
> Drop the self-recursion and have a helper function that does the actual work,
> and a loop calling that for each listener.
> Feel free to call the helper function directly, or through a different
function
> if you want the asserts outside the loop, if you know you have only a single
> listener.

created helper function.

[floitsch, 2013-09-11 12:02:57]

Committed patchset #7 manually as r27375 (presubmit successful).

[floitsch, 2013-09-11 13:34:34]

PTAL. needed to fix some asserts.

[Lasse Reichstein Nielsen, 2013-09-11 13:38:07]

lgtm

[floitsch, 2013-09-11 14:09:30]

Committed patchset #9 manually as r27383 (presubmit successful).