Stream isolates.

Stream isolates.

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

[floitsch, 2012-11-21 22:06:21]

Somehow the isolate_stream.dart file doesn't get access to "spawnUri" and
"port", so the CL is not yet functional.

[Lasse Reichstein Nielsen, 2012-11-22 10:23:15]

LGTM if you fix references.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
File sdk/lib/isolate/isolate_stream.dart (right):

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:11: final IsolateStream stream = new
IsolateStream._fromOriginalReceivePort(port);
Is 'port' defined in base.dart?

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:14: * A [MessageBox] creates an
[IsolateStream] [stream] and an [IsolateSink]
Commas around [stream] and [sink].

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:36: const int _ISOLATE_STREAM_TOKEN =
132421112;
Pah, doesn't look random enough :P

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:46: StreamController _controller = new
StreamController();
The base StreamController doesn't handle 'pause' at all (i.e., it receives
requests to pause, but does nothing).
I assume this is sufficient here.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:137: // TODO(floitsch): what should happen
when an IsolateSink is closed?
You are sending the 'done' event to the other end, and you can't send more
events on this end. You can do whatever is consistent with that.
The big question is ofcourse whether the associated stream is also closed, but
the other end should be deciding that - it's the one who can close that stream.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:156: * arguments. It is illegal to pass a
function closure.
"closure" isn't defined. The previous two cases define the valid cases well
enough, so just stop after that.
(If you want to elaborate, say that a top-level function is a declared function,
not just a function expression living at the top level).

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.dart
File sdk/lib/isolate/mangler.dart (right):

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:23: if (_encoded.containsKey(data)) return
_encoded[data];
This uses equality, not identiity. We should use an IdentityMap for this (when
we have one).

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:46: }
hasBeenDuplicated = true; ?

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:57: if (encoded != entry) needsCopy = true;
and break. No need to test more after the first modifying encoding was hit.
If you want to avoid duplicate encoding, you can even store entry and encoded,
and don't bother trying to encode until after you've seen entry in the next
iteration.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:72: if (encodedValue != value) needsCopy = true;
And break after the first encoding is found.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:83: _visiting.remove(data);
Consider keeping it and throwing if you see the same data again. Unless the
mangling can give different results for the same data, you're in an infinite
recursion.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:101: _createReference() => [manglingToken,
_REFERENCE];
What is it a reference to? I.e., how can you tell two references apart, if you
have multiple self-referencing objects?
Try sending:
 var x = [1,2,[4,5],3];
 x[2].add(x);
 x[2].add(x[2]);
and see what you get when mangling/unmangling.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:109: return [manglingToken, _ESCAPED, list];
Then the list contents are not encoded. You should encode the remaining list
elements into this list, effectively only replacing the first element with two
elements (and reverse when decoding ofcourse).

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:133: if (_decoded.containsKey(data)) return
_decoded[data];
if 'data' comes from a JSON.parse, it's not equal to something with the same
structure. When will this match?

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:138: return _decodeObject(declared, reference);
So the reference is just a list: [magic#, REFERENCE].
It's equality is identity equality. How will that be used?

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:145: if (_decoded.containsKey(data)) {
I don't think this lookup will ever succeed if the data has been JSON
stringified and then parsed.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:151: // map.
I find that unlikely.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:161: if (_isEscaped(data)) data =
_extractEscaped(data);
Here you decode the escaped data, even if it wasn't encoded.
Except that you don't get here because _isMangled returns true on escapes?

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:191: if (decoded != entry) needsCopy = true;
and break.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:228: => data is List && !data.isEmpty && data[0] ==
manglingToken;
'!data.isEmpty' -> 'data.length >= 2' or you can't safely test data[1] in the
following tests. 
Or is that an assert? If a list contains manglingToken as first, it's never a
single-element list, because then it would be escaped anyway?

[floitsch, 2012-11-22 11:58:04]

Looping in Anton and Pavel (to get a look from an embedder's point of view).

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
File sdk/lib/isolate/isolate_stream.dart (right):

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:11: final IsolateStream stream = new
IsolateStream._fromOriginalReceivePort(port);
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> Is 'port' defined in base.dart?

yes.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:14: * A [MessageBox] creates an
[IsolateStream] [stream] and an [IsolateSink]
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> Commas around [stream] and [sink].

Done.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:36: const int _ISOLATE_STREAM_TOKEN =
132421112;
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> Pah, doesn't look random enough :P

Changed to 132421119. ;)

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:46: StreamController _controller = new
StreamController();
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> The base StreamController doesn't handle 'pause' at all (i.e., it receives
> requests to pause, but does nothing).
> I assume this is sufficient here.

It is (I think), but iirc when I started this CL the pausing Streams weren't in
yet.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:137: // TODO(floitsch): what should happen
when an IsolateSink is closed?
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> You are sending the 'done' event to the other end, and you can't send more
> events on this end. You can do whatever is consistent with that.
> The big question is ofcourse whether the associated stream is also closed, but
> the other end should be deciding that - it's the one who can close that
stream.

We will discuss this in person.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:156: * arguments. It is illegal to pass a
function closure.
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> "closure" isn't defined. The previous two cases define the valid cases well
> enough, so just stop after that.
> (If you want to elaborate, say that a top-level function is a declared
function,
> not just a function expression living at the top level).

Done.

[floitsch, 2012-11-22 20:06:12]

Still missing some tests, but close.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.dart
File sdk/lib/isolate/mangler.dart (right):

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:23: if (_encoded.containsKey(data)) return
_encoded[data];
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> This uses equality, not identiity. We should use an IdentityMap for this (when
> we have one).

Added TODO.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:46: }
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> hasBeenDuplicated = true; ?

Done.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:57: if (encoded != entry) needsCopy = true;
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> and break. No need to test more after the first modifying encoding was hit.
> If you want to avoid duplicate encoding, you can even store entry and encoded,
> and don't bother trying to encode until after you've seen entry in the next
> iteration.

Duplicate encoding is not a problem since they are cached anyways. (in the
_encoded map).
done for the break.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:72: if (encodedValue != value) needsCopy = true;
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> And break after the first encoding is found.

cannot. This is a map.forEach.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:83: _visiting.remove(data);
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> Consider keeping it and throwing if you see the same data again. Unless the
> mangling can give different results for the same data, you're in an infinite
> recursion.

Added TODO.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:101: _createReference() => [manglingToken,
_REFERENCE];
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> What is it a reference to? I.e., how can you tell two references apart, if you
> have multiple self-referencing objects?
> Try sending:
>  var x = [1,2,[4,5],3];
>  x[2].add(x);
>  x[2].add(x[2]);
> and see what you get when mangling/unmangling.

Done.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:109: return [manglingToken, _ESCAPED, list];
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> Then the list contents are not encoded. You should encode the remaining list
> elements into this list, effectively only replacing the first element with two
> elements (and reverse when decoding ofcourse).

_escapeIfNecessary is only called on a encoded list.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:133: if (_decoded.containsKey(data)) return
_decoded[data];
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> if 'data' comes from a JSON.parse, it's not equal to something with the same
> structure. When will this match?

data comes from the isolate-communication. These guarantee that DAGs stay DAGs.
In other words: we can get the same list (or any other instance) multiple times.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:138: return _decodeObject(declared, reference);
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> So the reference is just a list: [magic#, REFERENCE].
> It's equality is identity equality. How will that be used?

This works because of the guarantees of the send/receive port communication.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:145: if (_decoded.containsKey(data)) {
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> I don't think this lookup will ever succeed if the data has been JSON
> stringified and then parsed.

which it hasn't.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:151: // map.
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> I find that unlikely.

see above.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:161: if (_isEscaped(data)) data =
_extractEscaped(data);
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> Here you decode the escaped data, even if it wasn't encoded.
> Except that you don't get here because _isMangled returns true on escapes?

data cannot be mangled and escaped at the same time.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:191: if (decoded != entry) needsCopy = true;
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> and break.

Done.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:228: => data is List && !data.isEmpty && data[0] ==
manglingToken;
On 2012/11/22 10:23:15, Lasse Reichstein Nielsen wrote:
> '!data.isEmpty' -> 'data.length >= 2' or you can't safely test data[1] in the
> following tests. 
> Or is that an assert? If a list contains manglingToken as first, it's never a
> single-element list, because then it would be escaped anyway?

added assert.

[Lasse Reichstein Nielsen, 2012-11-23 12:12:38]

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.dart
File sdk/lib/isolate/mangler.dart (right):

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:72: if (encodedValue != value) needsCopy = true;
True. You would need to iterate the keys instead.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:109: return [manglingToken, _ESCAPED, list];
Ah, true.

https://codereview.chromium.org/11308154/diff/2001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:161: if (_isEscaped(data)) data =
_extractEscaped(data);
Ack, isMangled is not the same as isMarked.

[Anton Muhin, 2012-11-25 10:23:16]

DBCs

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
File sdk/lib/isolate/isolate_stream.dart (right):

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:44: class IsolateStream extends
Stream<dynamic> {
just for my education, what's the benefit of Stream<dynamic> over just Stream?

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:83: dynamic _unmangleMessage(var message) {
why dynamic return type, not var or just dropped

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:133: (data) => (data is IsolateSink) ?
["Sink", data._port] : data);
I am sure you took care of it, but what happens if I pass ["Sink", port] in
write?

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/mangler.dart
File sdk/lib/isolate/mangler.dart (right):

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:8: final Map _encoded = new Map();
why not = {}?

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:16: _IsolateEncoder(this.manglingToken,
mangle(data))
why not this._mangle here?

https://codereview.chromium.org/11308154/diff/7001/tests/isolate/stream_mangl...
File tests/isolate/stream_mangling_test.dart (right):

https://codereview.chromium.org/11308154/diff/7001/tests/isolate/stream_mangl...
tests/isolate/stream_mangling_test.dart:15: Expect.identical(nested, nested[0]);
again, for my education, why do you use old Expect stuff here?

[Ivan Posva, 2012-11-26 18:56:40]

DBC -ip

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/base.dart
File sdk/lib/isolate/base.dart (right):

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/base.dart#...
sdk/lib/isolate/base.dart:146: // The VM doesn't support accessing external
globals in the same library. We
Can you please elaborate what is missing from our tests? There should be no
reason to not be able to access external from the library.

[floitsch, 2012-11-28 14:13:17]

TODO is updated in https://codereview.chromium.org//11416240

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/base.dart
File sdk/lib/isolate/base.dart (right):

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/base.dart#...
sdk/lib/isolate/base.dart:146: // The VM doesn't support accessing external
globals in the same library. We
On 2012/11/26 18:56:40, Ivan Posva wrote:
> Can you please elaborate what is missing from our tests? There should be no
> reason to not be able to access external from the library.

We cannot access external global variables from within the same library. The VM
will just not see it.
You can test this by adding a ReceivePort get port2 => port in the original code
and try to access it (after having imported dart:async).
Filed issue 6997 and updated TODO with bug-number.

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
File sdk/lib/isolate/isolate_stream.dart (right):

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:44: class IsolateStream extends
Stream<dynamic> {
On 2012/11/25 10:23:16, Anton Muhin wrote:
> just for my education, what's the benefit of Stream<dynamic> over just Stream?

Semantically it is the same. We (I?) prefer to write it explicitly to show that
I thought of the generic type and didn't just forget it.

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:83: dynamic _unmangleMessage(var message) {
On 2012/11/25 10:23:16, Anton Muhin wrote:
> why dynamic return type, not var or just dropped

ditto. To show that I thought about it.

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/isolate_st...
sdk/lib/isolate/isolate_stream.dart:133: (data) => (data is IsolateSink) ?
["Sink", data._port] : data);
On 2012/11/25 10:23:16, Anton Muhin wrote:
> I am sure you took care of it, but what happens if I pass ["Sink", port] in
> write?

The Encoder/Decoder takes care of this. If the mangler returns an object that is
different from the original it will mark it. Since a message ["Sink", port]
wouldn't be marked it wouldn't go through the demangler.

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/mangler.dart
File sdk/lib/isolate/mangler.dart (right):

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:8: final Map _encoded = new Map();
On 2012/11/25 10:23:16, Anton Muhin wrote:
> why not = {}?
If I'm not wrong that would be a Map<String, dynamic> and not Map<dynamic,
dynamic>.

https://codereview.chromium.org/11308154/diff/7001/sdk/lib/isolate/mangler.da...
sdk/lib/isolate/mangler.dart:16: _IsolateEncoder(this.manglingToken,
mangle(data))
On 2012/11/25 10:23:16, Anton Muhin wrote:
> why not this._mangle here?

This way I get a little bit of typing. The field is just declared as Function,
whereas the parameter is defined as a function that takes exactly one argument.

https://codereview.chromium.org/11308154/diff/7001/tests/isolate/stream_mangl...
File tests/isolate/stream_mangling_test.dart (right):

https://codereview.chromium.org/11308154/diff/7001/tests/isolate/stream_mangl...
tests/isolate/stream_mangling_test.dart:15: Expect.identical(nested, nested[0]);
On 2012/11/25 10:23:16, Anton Muhin wrote:
> again, for my education, why do you use old Expect stuff here?

Two reasons:
* I have never used the new unit-testing framework. (That could be fixed ;)
* for basic tests (in particular the language tests) we don't want to rely on
complicated unit-test frameworks. We want the tests to fail, and not the
framework before it can even execute our tests.
For isolate tests we could however consider using the framework.