Very simple version of Isolates.

Very simple version of Isolates.

R=ajohnsen@google.com, iposva@google.com, kasperl@google.com, lrn@google.com

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

[floitsch, 2013-10-14 17:32:28]

Fyi. I'm working towards a really simple version of the dart:isolate library.
I'm not done yet! (This is not yet a review request.)

This CL just implements the initial-message passing for spawnFunction, so that I
can continue on the actual API. Fwiw the following example works already:

===
import 'dart:isolate';

foo(port) {
  print("foo");
  port.send("foo");
}

bar() { print("bar"); }

main() {
  ReceivePort port = new ReceivePort();
  Isolate.spawn(foo, port.sendPort).then((_) { print("spawned"); });
  port.first.then(print);
  Isolate.spawn(bar, null);
}
===
foo
bar
spawned
foo
===

[kasperl, 2013-10-15 10:26:11]

I'd simplify even further, but other than that the interface LGTM.

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:49: { bool startPaused: false });
How about just getting rid of this pausing support and then adding it when it
works?

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:60: Isolate._fromControlPort(this._controlPort);
I'd move this up above the static functions.

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:62: /**
I'd get rid of these unimplemented methods and then add them as you implement
them.

[Lasse Reichstein Nielsen, 2013-10-17 12:26:54]

https://codereview.chromium.org/27215002/diff/21001/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/21001/sdk/lib/isolate/isolate.d...
sdk/lib/isolate/isolate.dart:50: external static Future<Isolate> spawn(void
entryPoint(message), var message);
Documentation says: "takes no arguments". This requires it to take arguments.
How about allowing it to take no arguments (Function typed, documented as taking
zero or one arguments), and make message optional?

[floitsch, 2013-10-17 12:30:55]

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:49: { bool startPaused: false });
On 2013/10/15 10:26:11, kasperl wrote:
> How about just getting rid of this pausing support and then adding it when it
> works?

Done.

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:60: Isolate._fromControlPort(this._controlPort);
On 2013/10/15 10:26:11, kasperl wrote:
> I'd move this up above the static functions.

Done.

https://codereview.chromium.org/27215002/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:62: /**
On 2013/10/15 10:26:11, kasperl wrote:
> I'd get rid of these unimplemented methods and then add them as you implement
> them.

Done.

https://codereview.chromium.org/27215002/diff/21001/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/21001/sdk/lib/isolate/isolate.d...
sdk/lib/isolate/isolate.dart:50: external static Future<Isolate> spawn(void
entryPoint(message), var message);
On 2013/10/17 12:26:54, Lasse Reichstein Nielsen wrote:
> Documentation says: "takes no arguments". This requires it to take arguments.
> How about allowing it to take no arguments (Function typed, documented as
taking
> zero or one arguments), and make message optional?

Yes. this came up yesterday.
Would be ok, and fundamentally this is what we do with spawnUri.

BUT: we will add named arguments to spawn and spawnUri. So just making it
optional won't work...

[floitsch, 2013-10-23 15:34:40]

@lrn: could you please start reviewing the test changes.
Even if we need to change the VM/patch files the tests should be fixed now.

[floitsch, 2013-10-23 22:11:57]

Sending out for review. Some parts are not polished yet (in particular the
actual Isolate.spawn and Isolate.spawnUri functions), but the tests have already
been adapted.
And they make the biggest chunk of this CL.

Anybody knows who is now responsible for utils/testrunner?
I couldn't find any tests for it.

Note: there is another pending CL for the isolate tests:
https://codereview.chromium.org/36933002/

Note2: the dart2js part is not yet done.

@johnmccutchan: the vmservice is currently broken (as discussed per mail). We
will need to fix it before landing this CL.

[Ivan Posva, 2013-10-24 06:48:38]

First round of comments...

-Ivan

https://codereview.chromium.org/27215002/diff/363001/runtime/bin/io_service_p...
File runtime/bin/io_service_patch.dart (right):

https://codereview.chromium.org/27215002/diff/363001/runtime/bin/io_service_p...
runtime/bin/io_service_patch.dart:35: subscription = _receivePort.listen((data)
{
This is one of those occasions where I would expect you to use the
RawReceivePort as you do not want to have to deal with streams, zones or
anything else when delivering the raw IO events from epoll to the IO subsystem.

https://codereview.chromium.org/27215002/diff/363001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/363001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:56: factory _RawReceivePortImpl() native
"ReceivePortImpl_factory";
"RawReceivePortImpl_factory"

https://codereview.chromium.org/27215002/diff/363001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/27215002/diff/363001/sdk/lib/io/timer_impl.da...
sdk/lib/io/timer_impl.dart:15: static StreamSubscription _subscription;
Where is this _subscription used?

[floitsch, 2013-10-24 16:15:56]

@Ivan: small overview of what this CL does and what still needs to be done.
I tried to minimize the changes to the c++ code. Eventually we will need to
modify the VM more, but the goal for this CL was to fix the user-facing API.
Current behind the scenes (new approach):
  * spawner spawns new isolate using old primitives.
  * VM doesn't launch real entry point but invokes _startIsolate with entrypoint
as argument.
  * _startIsolate communicates back to spawner and fetches the initial message
it has to deliver to the real entrypoint. The communication happens through the
global "port" variable, which still exists, but is hidden now.

Other partial changes: Sendport.send still takes a second "replyTo" argument,
but it can't be used from within Dart code anymore. I needed to keep the second
argument for the embedder's API. Currently only few objects can be exchanged
through that API and sendports are not yet part of it. I talked to Soeren and we
will soon change this. Then we can get rid of the second optional argument.

I removed the global exception handler (and the corresponding exception) from
the API. Internally it is still there.

https://codereview.chromium.org/27215002/diff/363001/runtime/bin/io_service_p...
File runtime/bin/io_service_patch.dart (right):

https://codereview.chromium.org/27215002/diff/363001/runtime/bin/io_service_p...
runtime/bin/io_service_patch.dart:35: subscription = _receivePort.listen((data)
{
On 2013/10/24 06:48:38, Ivan Posva wrote:
> This is one of those occasions where I would expect you to use the
> RawReceivePort as you do not want to have to deal with streams, zones or
> anything else when delivering the raw IO events from epoll to the IO
subsystem.

just wasn't changed back yet.
done.

https://codereview.chromium.org/27215002/diff/363001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/363001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:56: factory _RawReceivePortImpl() native
"ReceivePortImpl_factory";
On 2013/10/24 06:48:38, Ivan Posva wrote:
> "RawReceivePortImpl_factory"

Done.

https://codereview.chromium.org/27215002/diff/363001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/27215002/diff/363001/sdk/lib/io/timer_impl.da...
sdk/lib/io/timer_impl.dart:15: static StreamSubscription _subscription;
On 2013/10/24 06:48:38, Ivan Posva wrote:
> Where is this _subscription used?

spurious code from the ReceivePort -> RawReceivePort transition.
removed.

[Anders Johnsen, 2013-10-24 17:33:40]

IO stuff, LGTM

https://codereview.chromium.org/27215002/diff/493001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/27215002/diff/493001/sdk/lib/io/timer_impl.da...
sdk/lib/io/timer_impl.dart:166: _receivePort = new RawReceivePort((message) {
_handleTimeout(); });
(_) => _handleTimeout()

[floitsch, 2013-10-24 21:03:51]

https://codereview.chromium.org/27215002/diff/493001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/27215002/diff/493001/sdk/lib/io/timer_impl.da...
sdk/lib/io/timer_impl.dart:166: _receivePort = new RawReceivePort((message) {
_handleTimeout(); });
On 2013/10/24 17:33:41, Anders Johnsen wrote:
> (_) => _handleTimeout()

Done.

[Ivan Posva, 2013-10-25 07:01:44]

-Ivan

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:81: _portMap = new Map();
This does not need to be a DoublyLinkedHashMap, please change to HashMap.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:88: assert(_portMap != null);
These tests would not be needed if it was a static final Map _portMap = new
HashMap() below.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:115: void send(var message, [SendPort replyTo =
null]) {
As far as I understand the SendPort.send does not accept a replyTo anymore.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:119: void _sendNow(var message, SendPort replyTo)
{
This function makes no sense now. How about calling _sendInternal(_id, 0,
message) from send directly.

0 is an illegal/unmapped port id and then we'll remove the handling for replyTo
ports within the VM once this change has landed.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:129: return _id;
We should not leak the id out ever.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:162: Isolate._port.first.then((message) {
You cannot use first here. As far as I understand this will close the main port
of the isolate which makes the VM lose its "control" on the isolate.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:163: var reply = message[0];
Please assign the message[0] and message[1] values to name variables to document
what is going on.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:168: } else if (entryPoint is
_MainFunctionArgsMessage) {
This will not mix the if-statement for spawnFunction and spawnUri with the
dispatch based on arguments in the spawnUri case:

} else {
  // In this case we are spawning based on a spawnUri.
  assert(message.length == 3);
  if (entryPoint is _MainFunctionArgsMesssage) {
...

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:169: entryPoint(message[1], message[2]);
You are not checking whether the message even has 3 values.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:187: readyPort.handler = (_) {
... = (readyMsg) {
  assert(readyMsg == "started");
...

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:189: completer.complete(new
Isolate._fromControlPort(controlPort));
See note above. By the time you get here the controlPort has been closed on the
started isolate.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:206: readyPort.handler = (_) {
ditto

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:513: bool is_function_spawn = state->library_url() !=
NULL;
I find this is rather brittle and would suggest that you keep the fact whether
this was spawnFunction or spawnUri in an IsolateSpawnState::was_spawn_uri()
method. Within that method it is OK to rely on the above check, but then it is
all encapsulated within the IsolateSpawnState.

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:525: // Instead of directly invoking the entry point we
call '_startIsolate' with
This trampoline through _startIsolate will have the effect that this function
will be shown in the stack traces for spawned isolates. I am not concerned about
exposing this, but I just wanted to mention it.

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:1007: script_url_ =
strdup(GetRootScriptUri(Isolate::Current()));
There have been changes in this area, so you'll need to update for that. But I
expect that they should not have any immediate impact, but it would be good for
you to verify.

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:100: * The second argument [replyTo] is deprecated.
If this is being deprecated now, then that should mean we should just remove it.
Especially since there is no deprecated way at getting at this parameter from
the receiver's side.

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:211: // TODO(floitsch): probably going to remove
and replace with something else.
I would think we can safely remove this code for now as it is not properly
hooked up and it makes some of the other code much harder to follow. Once we
know how we want to propagate errors our, we can add the corrected handling back
into the isolate API and its implementations.

[Lasse Reichstein Nielsen, 2013-10-25 09:42:49]

Tests generally lgtm.

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/descriptor/async_test.dart:14: initialMessage = message;
What is "initialMessage"?
Appears to be a global variable in metatest.dart, but it's not obvious what it's
used for. 
A comment maybe, even if just // used by metatest.dart.

Or perhaps make a function so you can call:
  metaTestInit(message);
instead, or rename the "initialMessage" to something that signals what it's used
for.

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/metatest.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:110: /// The initial message the isolate
received.
the isolate received -> received by the isolate.

https://codereview.chromium.org/27215002/diff/553001/pkg/serialization/test/s...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/serialization/test/s...
pkg/serialization/test/serialization_test.dart:553: var remote =
Isolate.spawn(echo, [port.sendPort, output]);
I generally put the reply port last, just as it used to be the second argument
of the receive function.
But it's arbitrary anyway.

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/io_service_p...
File runtime/bin/io_service_patch.dart (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/io_service_p...
runtime/bin/io_service_patch.dart:34: _receivePort.handler = (data) {
Better! :)

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:875: // First try with 1 argument.
Could we inspect the function to see its signature, instead of trying to invoke
and failing? Or is trying to invoke efficient enough that it's not an overhead
(with the TODO below fixed)?

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:884: // Try with two arguments.
Say why we try two before zero.

We really want to pass the first argument (which we have), and don't care about
passing the second (which we don't have anyway).

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:218: DEFINE_NATIVE_ENTRY(isolate_spawnFunction, 1) {
We don't want to put the message through this, e.g., putting it into
IsolateSpawnState? (yet?)

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:233: "spawnFunction expects to be passed a closure to a
top-level static "
top-level and static are mutually exclusive. We may want to remove "static"
here.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:18: * event is received.
How about not making the handler optional?

You can still pass 'null' if you really, really want to (e.g., initializing a
final port field in a constructor, but setting the 
handler to an instance method).

Or making it default to something that throws, so it's not the port itself that
has to care about nulls.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:93: static void
_handleMessage(_RawReceivePortImpl port, int replyId, var message) {
long line.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:129: return _id;
The value needs to deterministically depend on _id only, so we'd need some sort
of hash of the _id instead.
How about a single "Jenkins" step?
    const int MASK = 0x3FFFFFFF; 
    int hash = _id;
    hash = (hash + ((hash & (MASK >> 10)) << 10)) & MASK;
    hash ^= (hash >> 6);
    hash = (hash + ((hash & (MASK >> 3)) << 3)) & MASK;
    hash ^= (hash >> 11);
    hash = (hash + ((hash & (MASK >> 15)) << 15)) & MASK;
    return hash;

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:3678: "  reply.send('echoing ${msg(1)}}');\n"
msg->data?
Or perhaps: msg(1) => data.i
As long as the test is successful, this code is never reached, so it probably
never worked.

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:100: * The second argument [replyTo] is deprecated.
Agree. We have broken almost everything else about isolates, so breaking
SendPort.send too isn't a big deal.
Any code that uses it will also assume that it works, which it doesn't, so it
might as well break early.

https://codereview.chromium.org/27215002/diff/553001/tools/dom/src/Isolates.dart
File tools/dom/src/Isolates.dart (right):

https://codereview.chromium.org/27215002/diff/553001/tools/dom/src/Isolates.d...
tools/dom/src/Isolates.dart:63: class SendPortSync {}
If this is just a marker interface, make it abstract.

https://codereview.chromium.org/27215002/diff/553001/utils/testrunner/testrun...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/27215002/diff/553001/utils/testrunner/testrun...
utils/testrunner/testrunner.dart:263: // Get the names of the source and target
test files and containing
Indentation seems wrong. It should be two characters relative to whatever the
.then is four characters indented from.

[floitsch, 2013-10-25 13:11:00]

PatchSet 27 addresses most of the current comments.

One comment left: remove more of the uncaught-exception support. I will look at
it separately.

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/descriptor/async_test.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/descriptor/async_test.dart:14: initialMessage = message;
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> What is "initialMessage"?
> Appears to be a global variable in metatest.dart, but it's not obvious what
it's
> used for. 
> A comment maybe, even if just // used by metatest.dart.
> 
> Or perhaps make a function so you can call:
>   metaTestInit(message);
> instead, or rename the "initialMessage" to something that signals what it's
used
> for.

Done.

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/metatest.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/metatest.dart:110: /// The initial message the isolate
received.
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> the isolate received -> received by the isolate.

Done.

https://codereview.chromium.org/27215002/diff/553001/pkg/serialization/test/s...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/serialization/test/s...
pkg/serialization/test/serialization_test.dart:553: var remote =
Isolate.spawn(echo, [port.sendPort, output]);
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> I generally put the reply port last, just as it used to be the second argument
> of the receive function.
> But it's arbitrary anyway.

I usually do too. Don't know why I didn't do it here.
done.

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/io_service_p...
File runtime/bin/io_service_patch.dart (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/io_service_p...
runtime/bin/io_service_patch.dart:34: _receivePort.handler = (data) {
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> Better! :)

.

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:875: // First try with 1 argument.
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> Could we inspect the function to see its signature, instead of trying to
invoke
> and failing? Or is trying to invoke efficient enough that it's not an overhead
> (with the TODO below fixed)?

It will be fixed. The TODO below is from Ivan. This is just a temporary
solution.

https://codereview.chromium.org/27215002/diff/553001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:884: // Try with two arguments.
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> Say why we try two before zero.
> 
> We really want to pass the first argument (which we have), and don't care
about
> passing the second (which we don't have anyway).

See comment above.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:218: DEFINE_NATIVE_ENTRY(isolate_spawnFunction, 1) {
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> We don't want to put the message through this, e.g., putting it into
> IsolateSpawnState? (yet?)

Eventually yes. But that involves serializing the message here, ...
The current approach was easier to do.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:233: "spawnFunction expects to be passed a closure to a
top-level static "
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> top-level and static are mutually exclusive. We may want to remove "static"
> here.

And the string was not accurate anymore..
Rewrote: Isolate.spawn expects to be passed a top-level function

This is actually a bug: it should allow static functions too.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:18: * event is received.
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> How about not making the handler optional?
> 
> You can still pass 'null' if you really, really want to (e.g., initializing a
> final port field in a constructor, but setting the 
> handler to an instance method).
> 
> Or making it default to something that throws, so it's not the port itself
that
> has to care about nulls.

The port doesn't care. It just tries to call it. If it's null, bad luck...

Willing to discuss.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:81: _portMap = new Map();
On 2013/10/25 07:01:46, Ivan Posva wrote:
> This does not need to be a DoublyLinkedHashMap, please change to HashMap.

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:88: assert(_portMap != null);
On 2013/10/25 07:01:46, Ivan Posva wrote:
> These tests would not be needed if it was a static final Map _portMap = new
> HashMap() below.

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:93: static void
_handleMessage(_RawReceivePortImpl port, int replyId, var message) {
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> long line.

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:115: void send(var message, [SendPort replyTo =
null]) {
On 2013/10/25 07:01:46, Ivan Posva wrote:
> As far as I understand the SendPort.send does not accept a replyTo anymore.

As discussed. We still need it for Native C communication.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:119: void _sendNow(var message, SendPort replyTo)
{
On 2013/10/25 07:01:46, Ivan Posva wrote:
> This function makes no sense now. How about calling _sendInternal(_id, 0,
> message) from send directly.
> 
> 0 is an illegal/unmapped port id and then we'll remove the handling for
replyTo
> ports within the VM once this change has landed.

ditto.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:129: return _id;
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> The value needs to deterministically depend on _id only, so we'd need some
sort
> of hash of the _id instead.
> How about a single "Jenkins" step?
>     const int MASK = 0x3FFFFFFF; 
>     int hash = _id;
>     hash = (hash + ((hash & (MASK >> 10)) << 10)) & MASK;
>     hash ^= (hash >> 6);
>     hash = (hash + ((hash & (MASK >> 3)) << 3)) & MASK;
>     hash ^= (hash >> 11);
>     hash = (hash + ((hash & (MASK >> 15)) << 15)) & MASK;
>     return hash;

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:129: return _id;
On 2013/10/25 07:01:46, Ivan Posva wrote:
> We should not leak the id out ever.

Implemented the single Jenkins hash step as suggested by Lasse.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:162: Isolate._port.first.then((message) {
On 2013/10/25 07:01:46, Ivan Posva wrote:
> You cannot use first here. As far as I understand this will close the main
port
> of the isolate which makes the VM lose its "control" on the isolate.

As discussed: closing the port for now.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:163: var reply = message[0];
On 2013/10/25 07:01:46, Ivan Posva wrote:
> Please assign the message[0] and message[1] values to name variables to
document
> what is going on.

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:168: } else if (entryPoint is
_MainFunctionArgsMessage) {
On 2013/10/25 07:01:46, Ivan Posva wrote:
> This will not mix the if-statement for spawnFunction and spawnUri with the
> dispatch based on arguments in the spawnUri case:
> 
> } else {
>   // In this case we are spawning based on a spawnUri.
>   assert(message.length == 3);
>   if (entryPoint is _MainFunctionArgsMesssage) {
> ...

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:169: entryPoint(message[1], message[2]);
On 2013/10/25 07:01:46, Ivan Posva wrote:
> You are not checking whether the message even has 3 values.

It would throw otherwise.
I have added the assert as suggest.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:187: readyPort.handler = (_) {
On 2013/10/25 07:01:46, Ivan Posva wrote:
> ... = (readyMsg) {
>   assert(readyMsg == "started");
> ...

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:189: completer.complete(new
Isolate._fromControlPort(controlPort));
On 2013/10/25 07:01:46, Ivan Posva wrote:
> See note above. By the time you get here the controlPort has been closed on
the
> started isolate.

Agreed.
As discussed. Leaving as is. Currently nobody accesses the control port.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:206: readyPort.handler = (_) {
On 2013/10/25 07:01:46, Ivan Posva wrote:
> ditto

ditto.

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:3678: "  reply.send('echoing ${msg(1)}}');\n"
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> msg->data?
> Or perhaps: msg(1) => data.i
> As long as the test is successful, this code is never reached, so it probably
> never worked.

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:513: bool is_function_spawn = state->library_url() !=
NULL;
On 2013/10/25 07:01:46, Ivan Posva wrote:
> I find this is rather brittle and would suggest that you keep the fact whether
> this was spawnFunction or spawnUri in an IsolateSpawnState::was_spawn_uri()
> method. Within that method it is OK to rely on the above check, but then it is
> all encapsulated within the IsolateSpawnState.

Done.

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:525: // Instead of directly invoking the entry point we
call '_startIsolate' with
On 2013/10/25 07:01:46, Ivan Posva wrote:
> This trampoline through _startIsolate will have the effect that this function
> will be shown in the stack traces for spawned isolates. I am not concerned
about
> exposing this, but I just wanted to mention it.

Yes. It's something we can live with in the near future.
Eventually we want to remove the trampoline.

https://codereview.chromium.org/27215002/diff/553001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:1007: script_url_ =
strdup(GetRootScriptUri(Isolate::Current()));
On 2013/10/25 07:01:46, Ivan Posva wrote:
> There have been changes in this area, so you'll need to update for that. But I
> expect that they should not have any immediate impact, but it would be good
for
> you to verify.

Done.

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:100: * The second argument [replyTo] is deprecated.
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> Agree. We have broken almost everything else about isolates, so breaking
> SendPort.send too isn't a big deal.
> Any code that uses it will also assume that it works, which it doesn't, so it
> might as well break early.

We can't. It is used by the C API. We need to be able to serialize SendPorts
first. Soeren volunteered to do it soon.

https://codereview.chromium.org/27215002/diff/553001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:100: * The second argument [replyTo] is deprecated.
On 2013/10/25 07:01:46, Ivan Posva wrote:
> If this is being deprecated now, then that should mean we should just remove
it.
> Especially since there is no deprecated way at getting at this parameter from
> the receiver's side.

can't yet. See reply to Lasse.

https://codereview.chromium.org/27215002/diff/553001/tools/dom/src/Isolates.dart
File tools/dom/src/Isolates.dart (right):

https://codereview.chromium.org/27215002/diff/553001/tools/dom/src/Isolates.d...
tools/dom/src/Isolates.dart:63: class SendPortSync {}
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> If this is just a marker interface, make it abstract.

Marking as abstract now, but I'm not sure if we don't need to temporarily revive
this class.

https://codereview.chromium.org/27215002/diff/553001/utils/testrunner/testrun...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/27215002/diff/553001/utils/testrunner/testrun...
utils/testrunner/testrunner.dart:263: // Get the names of the source and target
test files and containing
On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> Indentation seems wrong. It should be two characters relative to whatever the
> .then is four characters indented from.

Done.

[floitsch, 2013-10-25 14:21:13]

PTAL. I merged the isolate tests, and the dart2js changes (which have been
reviewed here:
https://chromiumcodereview.appspot.com/43663003/
https://codereview.chromium.org/40023002/
https://codereview.chromium.org/36933002/ )

[Lasse Reichstein Nielsen, 2013-10-25 14:40:02]

Checked sporadically but LGTM.

https://codereview.chromium.org/27215002/diff/783001/sdk/lib/_internal/lib/is...
File sdk/lib/_internal/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/783001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_patch.dart:42: throw new
UnimplementedError("ReceivePort.fromRawReceivePort");
Any plans for this?

[floitsch, 2013-10-25 17:46:15]

@siva: I had to update tests/standalone/issue14236_source.dart

There were no instructions, but I think I got it right.

https://codereview.chromium.org/27215002/diff/783001/sdk/lib/_internal/lib/is...
File sdk/lib/_internal/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/783001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_patch.dart:42: throw new
UnimplementedError("ReceivePort.fromRawReceivePort");
On 2013/10/25 14:40:03, Lasse Reichstein Nielsen wrote:
> Any plans for this?

I don't think I will be able to address RawReceivePorts before committing. In
particular I wouldn't have any reviewer...

[Ivan Posva, 2013-10-25 18:41:37]

LGTM with a couple of comments.

-Ivan

https://codereview.chromium.org/27215002/diff/553001/pkg/serialization/test/s...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/27215002/diff/553001/pkg/serialization/test/s...
pkg/serialization/test/serialization_test.dart:553: var remote =
Isolate.spawn(echo, [port.sendPort, output]);
On 2013/10/25 13:11:01, floitsch wrote:
> On 2013/10/25 09:42:49, Lasse Reichstein Nielsen wrote:
> > I generally put the reply port last, just as it used to be the second
argument
> > of the receive function.
> > But it's arbitrary anyway.
> 
> I usually do too. Don't know why I didn't do it here.
> done.

I generally put the reply first. I guess this is because I think of it similarly
to networking.

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/553001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:129: return _id;
On 2013/10/25 13:11:01, floitsch wrote:
> On 2013/10/25 07:01:46, Ivan Posva wrote:
> > We should not leak the id out ever.
> 
> Implemented the single Jenkins hash step as suggested by Lasse.

As long as we cannot go from the hash back to the id, I am fine.

https://codereview.chromium.org/27215002/diff/783001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/783001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:106: // id two RawReceivePort mapping.
?

https://codereview.chromium.org/27215002/diff/783001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:215: if (args is List<String>) {
I strongly disagree on this point. It should be up to me an the spawnee to
determine what we are sending across?

This is as if Dart was checking the types of arguments before calling and not in
the method being called. Please remove.

[floitsch, 2013-10-25 18:55:53]

https://codereview.chromium.org/27215002/diff/783001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/27215002/diff/783001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:106: // id two RawReceivePort mapping.
On 2013/10/25 18:41:39, Ivan Posva wrote:
> ?

don't ask...
I guess I removed the "to" by accident and then didn't pay attention when I
added it back.
done.

https://codereview.chromium.org/27215002/diff/783001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:215: if (args is List<String>) {
On 2013/10/25 18:41:39, Ivan Posva wrote:
> I strongly disagree on this point. It should be up to me an the spawnee to
> determine what we are sending across?
> 
> This is as if Dart was checking the types of arguments before calling and not
in
> the method being called. Please remove.

The reason is that dart2js would otherwise need to assume that main(args) can
receive a sendport as argument which, in turn, would drag in all the isolate
code.

I will remove the code in the VM.
I think I will keep it in the dart2js version (since it has other benefits
there). I will create a bug to revisit the whole situation.

[floitsch, 2013-10-25 19:25:25]

Committed patchset #33 manually as r29271 (presubmit successful).