Make chained futures point to their source instead of opposite.

Make chained futures point to their source instead of opposite.

This means that a chained future that nobody cares about will
not be kept alive by the source.
Any listeners added to the chained future is forwarded to the
source. If the chained source notices that the source has completed,
it copies the values and drops the link completely.

This should allow some unused futures to be GC'ed earlier than
otherwise.

R=floitsch@google.com

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

[Lasse Reichstein Nielsen, 2015-12-11 06:53:18]

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

[floitsch, 2015-12-11 09:02:10]

LGTM!

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

https://codereview.chromium.org/1516783003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:181: bool get _mayAddListener    => _state <=
_PENDING_COMPLETE;
I prefer being explicit (_state == _INCOMPLETE || _STATE == _PENDING_COMPLETE),
but since there is already a ">=" below...

https://codereview.chromium.org/1516783003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:301: _FutureListener current =
_resultOrListeners;
I prefer "old", but "current" is ok.

https://codereview.chromium.org/1516783003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:474: assert(typedValue is T);  // Avoid warning
that typedValue is unused.
If I understand correctly, the assignment is just to have a check in checked
mode.
Then you might as well just leave the assert, and remove line 473.

[Lasse Reichstein Nielsen, 2015-12-11 10:13:51]

Description was changed from

==========
Make chained futures point to their source instead of opposite.

This means that a chained future that nobody cares about will
not be kept alive by the source.
Any listeners added to the chained future is forwarded to the
source. If the chained source notices that the source has completed,
it copies the values and drops the link completely.

This should allow some unused futures to be GC'ed earlier than
otherwise.
==========

to

==========
Make chained futures point to their source instead of opposite.

This means that a chained future that nobody cares about will
not be kept alive by the source.
Any listeners added to the chained future is forwarded to the
source. If the chained source notices that the source has completed,
it copies the values and drops the link completely.

This should allow some unused futures to be GC'ed earlier than
otherwise.

R=floitsch@google.com

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

[Lasse Reichstein Nielsen, 2015-12-11 10:13:53]

Committed patchset #2 (id:20001) manually as
ed0bc4315832315c76651e33cf02da2a0c5e713d (presubmit successful).

[floitsch, 2015-12-11 18:43:43]

Please send out your comments.

[sra1, 2015-12-11 19:19:46]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-12-11 19:19:47]

Can we make the code smaller? This adds 2k to swarm which afaik does not use
chained futures.

[Lasse Reichstein Nielsen, 2015-12-11 22:51:34]

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

https://codereview.chromium.org/1516783003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:181: bool get _mayAddListener    => _state <=
_PENDING_COMPLETE;
The body is optimized for performance, the name is added for readability.

https://codereview.chromium.org/1516783003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:301: _FutureListener current =
_resultOrListeners;
Changed to existingListeners.

https://codereview.chromium.org/1516783003/diff/1/sdk/lib/async/future_impl.d...
sdk/lib/async/future_impl.dart:474: assert(typedValue is T);  // Avoid warning
that typedValue is unused.
I could, but the error message is different.

[Lasse Reichstein Nielsen, 2015-12-11 22:52:19]

On 2015/12/11 19:19:47, sra1 wrote:
> Can we make the code smaller? This adds 2k to swarm which afaik does not use
> chained futures.

I have an idea that might drop the "_prependListeners" function. I can try that.

[floitsch, 2015-12-11 22:56:58]

On 2015/12/11 22:52:19, Lasse Reichstein Nielsen wrote:
> On 2015/12/11 19:19:47, sra1 wrote:
> > Can we make the code smaller? This adds 2k to swarm which afaik does not use
> > chained futures.
> 
> I have an idea that might drop the "_prependListeners" function. I can try
that.

I would be surprised if the 2k came from the new code (although it's possible).
It's more likely to me that the TI got confused by the new code. Could also be
the innocent-looking "x is T" assert (although that one shouldn't do anything in
release build).

[Lasse Reichstein Nielsen, 2015-12-11 23:18:46]

On 2015/12/11 22:52:19, Lasse Reichstein Nielsen wrote:
> On 2015/12/11 19:19:47, sra1 wrote:
> > Can we make the code smaller? This adds 2k to swarm which afaik does not use
> > chained futures.
> 
> I have an idea that might drop the "_prependListeners" function. I can try
that.

Florian might be right that this is not the source of the increase, but try
checking if https://codereview.chromium.org/1524533002 makes a difference for
Swarm.