Move service into VM

Split service into VM and embedder specific bits.
Move most service sources into the VM.

R=asiva@google.com

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

[Cutch, 2014-01-06 20:38:25]

Siva & Todd,

This is a WIP CL. The standalone VM is working and I will finish the Dartium
bits this week.

I'm interested in early feedback on the changes to the Dart API.

John

[Cutch, 2014-01-08 17:33:39]

PTAL with this CL as well: https://codereview.chromium.org/128733002/

[siva, 2014-01-10 00:01:54]

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

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:588: const char* script_uri = "dart:vmservice";
You could add a DartUtils::kVMServiceLibURL value for "dart:vmservice"

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:590: bool *is_compile_error = &is_compile_error_;
I don't see the variables is_compile_error and is_compile_error_ being used in
here?

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:618: Dart_Handle io_lib_url =
DartUtils::NewString("dart:io");
DartUtils::kIOLibURL instead of "dart:io"

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:625: Dart_GetType(io_lib, platform_class_name, 0, NULL);
You could use
Dart_Handle platform_type = DartUtils::GetDartType(DartUtils::kIOLibURL,
"Platform");

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:627: Dart_Handle script_name_name =
DartUtils::NewString("_nativeScript");
Why does the variable have to be called script_name_name and not just
script_name

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

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:58: static const char* get_resource_path(int idx)
{
These are not strictly accessors so you probably need to use camel case for the
name. How about just Path?

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:64: return entry->path_;
should we assert that entry_path_ is not NULL ?

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:67: static const char* get_resource_resource(int
idx) {
How about Resource for this.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:77: static ResourcesEntry* get_resource_entry(int
idx) {
Ditto Entry?

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:87: static ResourcesEntry* get_resources_table() {
service_bin_resources() or ResourcesTable for this ?

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:110: Dart_Isolate isolate =
Dart_GetServiceIsolate();
As discussed offline it would be great if we just did
Dart_Handle service_port = Dart_GetServicePort();
here and then just sent messages to the port. Not have to deal with starting a
thread etc..

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:637: *   being loaded in the created isolate.
Comment needs to be removed.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:692: char** error);
I am still debating whether it makes sense to have a new callback for creating
the service isolate or should we add an extra parameter to the regular create
isolate callback indicating a service isolate needs to be created. We can
discuss this with Todd.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:1104: DART_EXPORT bool Dart_IsSendPort(Dart_Handle
send_port);
Do we need this call if it is only to ensure that Dart_SendPortSend is only
called with valid send ports then I would remove it.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:1114: DART_EXPORT bool Dart_SendPortSend(Dart_Handle
send_port, Dart_Handle object);
Dart_SendPortSend seems like an odd name why not call it
Dart_PostMessage(Dart_Handle send_port, Dart_Handle message);

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:2466: DART_EXPORT Dart_Isolate
Dart_GetServiceIsolate(void* callback_data);
Per offline discussion we should move towards
Dart_GetServicePort();

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:2482: DART_EXPORT void Dart_UnregisterFromService();
Todd and I were discussing this, we were wondering if it makes sense for
isolates to be registered/unregistered by default and we could special case and
allow isolates to just unregister themselves if they did not want to be tracked
(We can even state that the embedder should filter out isolates that it does not
want tracked from the isolates list replies and we will always track all
isolates).
We can discuss this with Todd tomorrow.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl.cc:1184: DART_EXPORT bool Dart_SendPortSend(Dart_Handle
send_port, Dart_Handle object) {
Dart_Handle Dart_PostMessage(....)

This function should check if send_port is valid and return an error if it is
not a valid send port.

false returns from Dart_Post should also be returned as an error object.

on Success it would return Api::Success();

Maybe we should also change Dart_Post to operate like that.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:51: static const char* get_resource_path(int idx) {
ditto comments about the function names as they are not simple accessors.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:156: }
There is only one entry in the table does it need to be a loop? Do you think
there will more native entries later.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:179: }
What happens if multiple threads from the embedder call GetServiceIsolate? the
function is not MT safe

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:241: id ^= rp_id_obj.raw();
Since you are asserting that rp_id_obj is a Smi or Mint you
could do:
const Integer& id = Integer::Cast(rp_id_obj);

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:362: }
What is the conversion to C string library_url_string for? The variable is not
used anywhere in this tag handler.

library_url is also not used anywhere?

[Cutch, 2014-01-10 21:01:42]

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

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:588: const char* script_uri = "dart:vmservice";
On 2014/01/10 00:01:54, siva wrote:
> You could add a DartUtils::kVMServiceLibURL value for "dart:vmservice"

Done.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:590: bool *is_compile_error = &is_compile_error_;
On 2014/01/10 00:01:54, siva wrote:
> I don't see the variables is_compile_error and is_compile_error_ being used in
> here?

They are used in the CHECK_RESULT macro. I've added a comment.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:618: Dart_Handle io_lib_url =
DartUtils::NewString("dart:io");
On 2014/01/10 00:01:54, siva wrote:
> DartUtils::kIOLibURL instead of "dart:io"

Done here and elsewhere.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:625: Dart_GetType(io_lib, platform_class_name, 0, NULL);
On 2014/01/10 00:01:54, siva wrote:
> You could use
> Dart_Handle platform_type = DartUtils::GetDartType(DartUtils::kIOLibURL,
> "Platform");

Done here and elsewhere.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:627: Dart_Handle script_name_name =
DartUtils::NewString("_nativeScript");
On 2014/01/10 00:01:54, siva wrote:
> Why does the variable have to be called script_name_name and not just
> script_name

Done here and elsewhere.

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

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:67: static const char* get_resource_resource(int
idx) {
On 2014/01/10 00:01:54, siva wrote:
> How about Resource for this.

Killed it.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:77: static ResourcesEntry* get_resource_entry(int
idx) {
On 2014/01/10 00:01:54, siva wrote:
> Ditto Entry?

Renamed to "At".

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:87: static ResourcesEntry* get_resources_table() {
On 2014/01/10 00:01:54, siva wrote:
> service_bin_resources() or ResourcesTable for this ?

Done.

https://codereview.chromium.org/125103004/diff/40001/runtime/bin/vmservice_im...
runtime/bin/vmservice_impl.cc:110: Dart_Isolate isolate =
Dart_GetServiceIsolate();
On 2014/01/10 00:01:54, siva wrote:
> As discussed offline it would be great if we just did
> Dart_Handle service_port = Dart_GetServicePort();
> here and then just sent messages to the port. Not have to deal with starting a
> thread etc..

Agree that I should follow this cleanup with another which further moves the
service into the VM and out of the embedder.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:637: *   being loaded in the created isolate.
On 2014/01/10 00:01:54, siva wrote:
> Comment needs to be removed.

Done.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:1104: DART_EXPORT bool Dart_IsSendPort(Dart_Handle
send_port);
On 2014/01/10 00:01:54, siva wrote:
> Do we need this call if it is only to ensure that Dart_SendPortSend is only
> called with valid send ports then I would remove it.

Killed it.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:1114: DART_EXPORT bool Dart_SendPortSend(Dart_Handle
send_port, Dart_Handle object);
On 2014/01/10 00:01:54, siva wrote:
> Dart_SendPortSend seems like an odd name why not call it
> Dart_PostMessage(Dart_Handle send_port, Dart_Handle message);

Done.

https://codereview.chromium.org/125103004/diff/40001/runtime/include/dart_api...
runtime/include/dart_api.h:2482: DART_EXPORT void Dart_UnregisterFromService();
On 2014/01/10 00:01:54, siva wrote:
> Todd and I were discussing this, we were wondering if it makes sense for
> isolates to be registered/unregistered by default and we could special case
and
> allow isolates to just unregister themselves if they did not want to be
tracked
> (We can even state that the embedder should filter out isolates that it does
not
> want tracked from the isolates list replies and we will always track all
> isolates).
> We can discuss this with Todd tomorrow.

I've removed these methods for now and auto-register isolates. In the future if
we need to support unregistering, we can add them back.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:51: static const char* get_resource_path(int idx) {
On 2014/01/10 00:01:54, siva wrote:
> ditto comments about the function names as they are not simple accessors.

Done.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:156: }
On 2014/01/10 00:01:54, siva wrote:
> There is only one entry in the table does it need to be a loop? Do you think
> there will more native entries later.

There will be more natives later. Todd has a CL which adds another one.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:179: }
On 2014/01/10 00:01:54, siva wrote:
> What happens if multiple threads from the embedder call GetServiceIsolate? the
> function is not MT safe

Good point. Do we make thread safety guarantees about the API? What's to stop an
embedder from entering an isolate on two separate threads?

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:241: id ^= rp_id_obj.raw();
On 2014/01/10 00:01:54, siva wrote:
> Since you are asserting that rp_id_obj is a Smi or Mint you
> could do:
> const Integer& id = Integer::Cast(rp_id_obj);

Done.

https://codereview.chromium.org/125103004/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:362: }
On 2014/01/10 00:01:54, siva wrote:
> What is the conversion to C string library_url_string for? The variable is not
> used anywhere in this tag handler.
> 
> library_url is also not used anywhere?

Done.

[siva, 2014-01-14 18:18:59]

LGTM with comments.

I also added a comment about another possibility for the next round of
refactoring.

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

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:555: return isolate;
I think we should define CHECK_RESULT in the beginning of this function
and #undef it over here. It doesn't make sense to use the same macro in your new
function with a dummy is_compilation_error variable.

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:588: bool *is_compile_error = &is_compile_error_;
Instead of these dummy variables define a version of
CHECK_RESULT inside or at the beginning of this function and undef
it at the end.

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:633: }
Another point, this function above looks identical to
CreateIsolateAndSetupHelper with only script_uri being different and the isolate
being made runnable at the end. I am wondering it is possible to use the same
callback with script_uri set to DartUtils::kVMServiceLibURL

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:864: CreateServiceIsolate)) {
Another option for the next round of refactoring is to pass in the vmservice
script name that the embedder needs here instead of the createservice isolate
call.

That way the VM does the regular isolate create callback, loads the internal VM
service library and then does a LoadScript of this embedder specified script
here.

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:176: Isolate* Service::GetServiceIsolate(void*
callback_data) {
I am wondering if we should split this into
Service::InitOnce(void* callback_data) which is called from Dart::InitOnce();
and Service::GetServiceIsolate which only returns service_isolate_;

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:215: Dart_Handle url =
Dart_NewStringFromCString("dart:vmservice");
We have made symbols of other core dart libraries like
dart:core, dart:async etc..

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:217: Dart_Handle source =
Dart_NewStringFromCString(resource);
I think this may not be right to treat the dart:vmservice as a C string, see
bootstrap.cc we always use String::FromUTF8 for loading dart sources as the
source could contain some string literals which are not C string compatible.

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:247: return service_isolate_;
one general comment about this function, we are inside the VM you could probably
use the VM directly instead of using the Dart API calls.

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:275: return list;
Same comment about this function it could be plain VM code and not Dart API
calls.

[Cutch, 2014-01-14 19:53:25]

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

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:555: return isolate;
On 2014/01/14 18:19:00, siva wrote:
> I think we should define CHECK_RESULT in the beginning of this function
> and #undef it over here. It doesn't make sense to use the same macro in your
new
> function with a dummy is_compilation_error variable.

Done.

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:588: bool *is_compile_error = &is_compile_error_;
On 2014/01/14 18:19:00, siva wrote:
> Instead of these dummy variables define a version of
> CHECK_RESULT inside or at the beginning of this function and undef
> it at the end.

Done.

https://codereview.chromium.org/125103004/diff/190001/runtime/bin/main.cc#new...
runtime/bin/main.cc:864: CreateServiceIsolate)) {
On 2014/01/14 18:19:00, siva wrote:
> Another option for the next round of refactoring is to pass in the vmservice
> script name that the embedder needs here instead of the createservice isolate
> call.
> 
> That way the VM does the regular isolate create callback, loads the internal
VM
> service library and then does a LoadScript of this embedder specified script
> here.

Agreed, the next round of refactoring can clean this up further as discussed in
person (hide service isolate completely).

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:247: return service_isolate_;
On 2014/01/14 18:19:00, siva wrote:
> one general comment about this function, we are inside the VM you could
probably
> use the VM directly instead of using the Dart API calls.

Done.

https://codereview.chromium.org/125103004/diff/190001/runtime/vm/service.cc#n...
runtime/vm/service.cc:275: return list;
On 2014/01/14 18:19:00, siva wrote:
> Same comment about this function it could be plain VM code and not Dart API
> calls.

Done.

[Cutch, 2014-01-14 22:35:03]

Committed patchset #9 manually as r31809 (presubmit successful).