isolate lib: small refactor to distinguish protocols at the port level

isolate lib: small refactor to distinguish protocols at the port level

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

[eub, 2012-02-09 01:13:42]

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:331: final int _isolateId;
id of destination?

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:384: final shouldSerialize = _globalState.currentContext
== null;
(Revisit per our discussion -- is this test backwards?  and rewrite comment.)

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:399: (_receivePort == other._receivePort);
(Side note about Dart: this idiom strikes me as weird, because it allows
somebody else to subclass MyNativeJsSendPort, implement a similar operator==,
and end up with an asymmetric == relation.  But I see this routinely; maybe we
mean "don't subclass and do that then.")

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:436: return (_workerId << 16) ^ (_isolateId << 8) ^
_receivePortId;
If we believe these bit windows don't overlap, using | would document that.  If
we don't believe it (I don't think we do)... it really begs for a decent hash
mixing function.  TODO?

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate_ser...
File frog/lib/isolate_serialization.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate_ser...
frog/lib/isolate_serialization.dart:258: if (workerId ==
_globalState.currentWorkerId) {
Do we want to distinguish the serialization token string between the two types
of send ports instead of using this somewhat magic test?  Or is the magic better
since non-redundant?

[Siggi Cherem (dart-lang), 2012-02-10 00:16:41]

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:331: final int _isolateId;
On 2012/02/09 01:13:42, eub wrote:
> id of destination?

Yes. Added a comment.

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:384: final shouldSerialize = _globalState.currentContext
== null;
On 2012/02/09 01:13:42, eub wrote:
> (Revisit per our discussion -- is this test backwards?  and rewrite comment.)

cleaned up. Should make more sense now :)

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:399: (_receivePort == other._receivePort);
On 2012/02/09 01:13:42, eub wrote:
> (Side note about Dart: this idiom strikes me as weird, because it allows
> somebody else to subclass MyNativeJsSendPort, implement a similar operator==,
> and end up with an asymmetric == relation.  But I see this routinely; maybe we
> mean "don't subclass and do that then.")

yeah, it would be nice to have 'final' or 'sealed' classes :)

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate.dar...
frog/lib/isolate.dart:436: return (_workerId << 16) ^ (_isolateId << 8) ^
_receivePortId;
On 2012/02/09 01:13:42, eub wrote:
> If we believe these bit windows don't overlap, using | would document that. 
If
> we don't believe it (I don't think we do)... it really begs for a decent hash
> mixing function.  TODO?

Done.

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate_ser...
File frog/lib/isolate_serialization.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/5001/frog/lib/isolate_ser...
frog/lib/isolate_serialization.dart:258: if (workerId ==
_globalState.currentWorkerId) {
On 2012/02/09 01:13:42, eub wrote:
> Do we want to distinguish the serialization token string between the two types
> of send ports instead of using this somewhat magic test?  Or is the magic
better
> since non-redundant?

The magic is actually important for two reasons: check that we only create a
native port when we are in the correct worker (the redundancy issue you raised),
and normalize how ports are used. For the latter, we want to 'short-circuit'
ports that come back to the same worker. If I have 3 isolates, with A and B in
the same worker and C in a different one, then passing a port from A to C to B
should create a direct (nativejs) port B->A.

[eub, 2012-02-10 21:20:57]

LGTM, couple of small suggestions.

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.da...
frog/lib/isolate.dart:397: if (shouldSerialize) {
I liked your previous version with the serialize-deserialize in one shot instead
of two if() checks.

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate_se...
File frog/lib/isolate_serialization.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate_se...
frog/lib/isolate_serialization.dart:258: if (workerId ==
_globalState.currentWorkerId) {
Could you comment on the "port short circuiting" in the code?

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate_se...
frog/lib/isolate_serialization.dart:268: // TODO(floitsch): this should by
Map<int, var> or Map<int, Dynamic>.
So is this TODO done? :)

[Siggi Cherem (dart-lang), 2012-02-10 22:09:21]

Thanks Eli!

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.da...
frog/lib/isolate.dart:397: if (shouldSerialize) {
On 2012/02/10 21:20:57, eub wrote:
> I liked your previous version with the serialize-deserialize in one shot
instead
> of two if() checks.

Me too - when I did some testing, I realized that it didn't provide me the right
semantics. If I forced isolates to run in the same place, this example was
printing "after" instead of "before":

other(port) => port.receive((msg, _) => print(msg[0]));

main() {
  sample = ["before"];
  (new Isolate.fromCode(other)).sendPort.send(sample);
  sample[0] = "after";
}

Basically because the serialize-deserialize was applied after sample was
modified.

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate_se...
File frog/lib/isolate_serialization.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate_se...
frog/lib/isolate_serialization.dart:258: if (workerId ==
_globalState.currentWorkerId) {
On 2012/02/10 21:20:57, eub wrote:
> Could you comment on the "port short circuiting" in the code?

Done

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate_se...
frog/lib/isolate_serialization.dart:268: // TODO(floitsch): this should by
Map<int, var> or Map<int, Dynamic>.
On 2012/02/10 21:20:57, eub wrote:
> So is this TODO done? :)

Done.

[eub, 2012-02-10 22:11:49]

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.da...
frog/lib/isolate.dart:397: if (shouldSerialize) {
On 2012/02/10 22:09:22, sigmund wrote:
> On 2012/02/10 21:20:57, eub wrote:
> > I liked your previous version with the serialize-deserialize in one shot
> instead
> > of two if() checks.
> 
> Me too - when I did some testing, I realized that it didn't provide me the
right
> semantics. If I forced isolates to run in the same place, this example was
> printing "after" instead of "before":
> 
> other(port) => port.receive((msg, _) => print(msg[0]));
> 
> main() {
>   sample = ["before"];
>   (new Isolate.fromCode(other)).sendPort.send(sample);
>   sample[0] = "after";
> }
> 
> Basically because the serialize-deserialize was applied after sample was
> modified.

Can we come up with a simple comment explaining to my future self why I
shouldn't change the code to the other version? :)

[Siggi Cherem (dart-lang), 2012-02-10 22:14:14]

On 2012/02/10 22:11:49, eub wrote:
>
https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.dart
frog/lib/isolate.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9317068/diff/10001/frog/lib/isolate.da...
> frog/lib/isolate.dart:397: if (shouldSerialize) {
> On 2012/02/10 22:09:22, sigmund wrote:
> > On 2012/02/10 21:20:57, eub wrote:
> > > I liked your previous version with the serialize-deserialize in one shot
> > instead
> > > of two if() checks.
> > 
> > Me too - when I did some testing, I realized that it didn't provide me the
> right
> > semantics. If I forced isolates to run in the same place, this example was
> > printing "after" instead of "before":
> > 
> > other(port) => port.receive((msg, _) => print(msg[0]));
> > 
> > main() {
> >   sample = ["before"];
> >   (new Isolate.fromCode(other)).sendPort.send(sample);
> >   sample[0] = "after";
> > }
> > 
> > Basically because the serialize-deserialize was applied after sample was
> > modified.
> 
> Can we come up with a simple comment explaining to my future self why I
> shouldn't change the code to the other version? :)

will do - I'll add the test too :)