Use a _FutureListener separate from the _Future to hold listeners.

Use a _FutureListener separate from the _Future to hold listeners.

This separates the concerns, and makes _Future a three-field class.
Holding on to a future gets cheaper.

R=sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2014-10-08 10:59:13]

lrn@google.com changed reviewers:
+ sgjesse@google.com

[Lasse Reichstein Nielsen, 2014-10-08 10:59:14]

This is mainly refactoring to make the structures simpler.
Performance change is small on VM, with a slight improvement on values (5%) and
regression on whenComplete (-10%). An improvement on values is worth a lot,
since that's the common case. 

Dart2js performance is greatly improved across the board.

[Søren Gjesse, 2014-10-09 06:55:52]

lgtm

This is complicated code, but it seems to keep the same structure.

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

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:83: _FutureOnValue onValue, Function
errorCallback)
No typedef for errorCallback due to stack trace or not?

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:163: * The listeners is an internally linked list
of [_FutureListener]s.
So the _FutureListener class have existed before?

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:174: * one value at a time.
Please update this with the mapping to the state values - just refer to the
comments on the states above.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:207: result = new _Future();
Combine the declaration with first assignment.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:430: * If [runCallback] is true (which should be
the default) it executes
I don't see the runCallback argument.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:445: }
I think the comment about special-casing the one listener case still need a
comment here.

[Lasse Reichstein Nielsen, 2014-10-09 07:53:22]

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

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:83: _FutureOnValue onValue, Function
errorCallback)
Yes. It can be an either one- or two-argument function, so there is no common
supertype other than Function.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:163: * The listeners is an internally linked list
of [_FutureListener]s.
Yes, and the documentation wasn't updated when it was removed.
Back then there were four different subclasses of _FutureListener. The rewrite
made the dispatch loop monomorphic, and this change reintroduces a monomorphoc
future listener.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:174: * one value at a time.
On 2014/10/09 06:55:52, Søren Gjesse wrote:
> Please update this with the mapping to the state values - just refer to the
> comments on the states above.

Done.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:207: result = new _Future();
On 2014/10/09 06:55:51, Søren Gjesse wrote:
> Combine the declaration with first assignment.

Done.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:430: * If [runCallback] is true (which should be
the default) it executes
True. I think I removed that earlier, when I started clearing the callback
fields instead of using the flag.
Comment removed.

https://codereview.chromium.org/637893002/diff/20001/sdk/lib/async/future_imp...
sdk/lib/async/future_impl.dart:445: }
On 2014/10/09 06:55:52, Søren Gjesse wrote:
> I think the comment about special-casing the one listener case still need a
> comment here.

Done.

[Lasse Reichstein Nielsen, 2014-10-09 07:54:29]

Committed patchset #3 manually as r41006 (presubmit successful).