Update VM service to new isolate API

Update VM service to new isolate API

R=floitsch@google.com, iposva@google.com

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

[floitsch, 2013-10-24 15:28:30]

LGTM (but wait for Ivan).
Thanks!

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
runtime/bin/vmservice/vmservice.dart:22: ReceivePort _receivePort;
maybe:
final ReceivePort receivePort = new ReceivePort();

but I see why you would want to have the allocation and init (the ".receive")
together.

Maybe then:
final ReceivePort receivePort;

VMService._internal() : _receivePort = new ReceivePort() {
  _receivePort.receive(messageHandler);
}

[Ivan Posva, 2013-10-25 00:30:27]

-Ivan

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
runtime/bin/vmservice/vmservice.dart:43: port.close();
If you do not ever touch the default port, you should not need to close it. What
am I missing?

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
runtime/bin/vmservice/vmservice_io.dart:12: var _port;
Please add comments documenting what these ports are going to be used for and
where they are set up.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc
File runtime/bin/vmservice_impl.cc (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc...
runtime/bin/vmservice_impl.cc:150: Dart_Handle receivePort =
Dart_GetField(library,
I generally find this mix of external C API and internal VM functions confusing
and concerning. What is the functionality that was not available from the
external API?

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc...
runtime/bin/vmservice_impl.cc:180: result = Dart_CompileAll();
Please remove this or at least make it optional based on a flag.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc...
runtime/bin/vmservice_impl.cc:439: StackZone zone(isolate);
ditto.

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

https://codereview.chromium.org/38703009/diff/1/runtime/vm/isolate.cc#newcode389
runtime/vm/isolate.cc:389:
result->set_service_port(PortMap::CreatePort(result->message_handler()));
What is this service port used for? How does the VM account for the additional
open port when determining to terminate an isolate?

[Ivan Posva, 2013-10-25 00:31:02]

https://codereview.chromium.org/38703009/diff/1/tests/standalone/vmservice/is...
File tests/standalone/vmservice/isolate_stacktrace_command_script.dart (right):

https://codereview.chromium.org/38703009/diff/1/tests/standalone/vmservice/is...
tests/standalone/vmservice/isolate_stacktrace_command_script.dart:29:
port.close();
Why is this port.close() needed?

[Cutch, 2013-10-25 01:21:57]

Ivan: Patch Set 2 addresses all of your comments. Please see my comments about
the new isolate port.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
runtime/bin/vmservice/vmservice.dart:22: ReceivePort _receivePort;
On 2013/10/24 15:28:30, floitsch wrote:
> maybe:
> final ReceivePort receivePort = new ReceivePort();
> 
> but I see why you would want to have the allocation and init (the ".receive")
> together.
> 
> Maybe then:
> final ReceivePort receivePort;
> 
> VMService._internal() : _receivePort = new ReceivePort() {
>   _receivePort.receive(messageHandler);
> }

Done.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
runtime/bin/vmservice/vmservice.dart:43: port.close();
On 2013/10/25 00:30:27, Ivan Posva wrote:
> If you do not ever touch the default port, you should not need to close it.
What
> am I missing?

Removed.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice/vmservi...
runtime/bin/vmservice/vmservice_io.dart:12: var _port;
On 2013/10/25 00:30:27, Ivan Posva wrote:
> Please add comments documenting what these ports are going to be used for and
> where they are set up.

Done.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc
File runtime/bin/vmservice_impl.cc (right):

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc...
runtime/bin/vmservice_impl.cc:150: Dart_Handle receivePort =
Dart_GetField(library,
On 2013/10/25 00:30:27, Ivan Posva wrote:
> I generally find this mix of external C API and internal VM functions
confusing
> and concerning. What is the functionality that was not available from the
> external API?

The external API does not offer functionality to map from a ReceivePort or
SendPort to a Dart_Port.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc...
runtime/bin/vmservice_impl.cc:180: result = Dart_CompileAll();
On 2013/10/25 00:30:27, Ivan Posva wrote:
> Please remove this or at least make it optional based on a flag.

Done.

https://codereview.chromium.org/38703009/diff/1/runtime/bin/vmservice_impl.cc...
runtime/bin/vmservice_impl.cc:439: StackZone zone(isolate);
On 2013/10/25 00:30:27, Ivan Posva wrote:
> ditto.

Done.

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

https://codereview.chromium.org/38703009/diff/1/runtime/vm/isolate.cc#newcode389
runtime/vm/isolate.cc:389:
result->set_service_port(PortMap::CreatePort(result->message_handler()));
On 2013/10/25 00:30:27, Ivan Posva wrote:
> What is this service port used for? How does the VM account for the additional
> open port when determining to terminate an isolate?

(Florian is closing the main port from Dart code as part of his port/cleanup,
this was stopping the VM service from communicating with the isolate).

Using a separate port allows for Dart code to close the main port but still have
service messages delivered to isolates. 

The service port is only ever used as a SendPort (vm service sends OOB messages
over the SendPort) so it does not increment the live port count (except
momentarily when a service message is in flight and we allocate a ReceivePort)
and does not interfere with isolate shutdown.

https://codereview.chromium.org/38703009/diff/1/tests/standalone/vmservice/is...
File tests/standalone/vmservice/isolate_stacktrace_command_script.dart (right):

https://codereview.chromium.org/38703009/diff/1/tests/standalone/vmservice/is...
tests/standalone/vmservice/isolate_stacktrace_command_script.dart:29:
port.close();
On 2013/10/25 00:31:02, Ivan Posva wrote:
> Why is this port.close() needed?

Discussed offline, removed.

[Ivan Posva, 2013-10-29 03:35:50]

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice/vm...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice/vm...
runtime/bin/vmservice/vmservice.dart:22: final ReceivePort receivePort;
Are you sure you do not want a RawReceivePort? Or do you expect to use Zones and
other Stream based features? It would also remove the contortions you have to do
to get at the id, although I am not yet convinced we need it.

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice_im...
File runtime/bin/vmservice_impl.cc (right):

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:419: Dart_Handle list = Dart_NewList(4);
You only set 3 values in an array of 4 elements. It looks like the 4th element
is always expected, so you should pass it along here as well:
MakeServiceControlMessage(Dart_Port port, intptr_t code, Dart_Handle value)
{...} and call it with Dart_Null() if you do not have value such as in shutdown.

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:427: result = Dart_ListSetAt(list, 1,
port_id_handle);
Why are you sending both the port_id and the SendPort in this message?

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

https://codereview.chromium.org/38703009/diff/190001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:54: int get _id => _rawPort._id;
Please remove.

[Ivan Posva, 2013-10-29 04:18:24]

LGTM with comments addressed.

-Ivan

[Cutch, 2013-10-29 05:24:56]

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice/vm...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice/vm...
runtime/bin/vmservice/vmservice.dart:22: final ReceivePort receivePort;
Switched to RawReceivePort.

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice_im...
File runtime/bin/vmservice_impl.cc (right):

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:419: Dart_Handle list = Dart_NewList(4);
On 2013/10/29 03:35:50, Ivan Posva wrote:
> You only set 3 values in an array of 4 elements. It looks like the 4th element
> is always expected, so you should pass it along here as well:
> MakeServiceControlMessage(Dart_Port port, intptr_t code, Dart_Handle value)
> {...} and call it with Dart_Null() if you do not have value such as in
shutdown.

Done.

https://codereview.chromium.org/38703009/diff/190001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:427: result = Dart_ListSetAt(list, 1,
port_id_handle);
On 2013/10/29 03:35:50, Ivan Posva wrote:
> Why are you sending both the port_id and the SendPort in this message?

Discussed offline: port_id is the unique key that is used to identify the
isolate externally. Will keep this for now but replace with a different key in
the future once an isolate identifier encoding is fixed.

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

https://codereview.chromium.org/38703009/diff/190001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:54: int get _id => _rawPort._id;
On 2013/10/29 03:35:50, Ivan Posva wrote:
> Please remove.

Done.

[Cutch, 2013-10-29 06:12:42]

Committed patchset #5 manually as r29415 (presubmit successful).