Load service isolate from the kernel binary.

Load service isolate from a .dill file.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/b0c2a382ca70bc0f2270e4b8c7ccc76167f256cc

[sivachandra, 2017-06-08 00:09:42]

sivachandra@google.com changed reviewers:
+ asiva@google.com

[sivachandra, 2017-06-08 00:09:43]

This is not a request for review but to show the current implementation so that
I can get early feedback.

Few points:

* The service isolate's dill file is around 1500 bytes larger than the
platform.dill file.

* Peter Ahe asked me to file this bug:
https://github.com/dart-lang/sdk/issues/29818. So, if this issue is fixed, we
can actually pack the service isolate kernel with the dart exe as it is being
done in this change (We will additionally have to load platform.dill in the
service isolate, similar to how it is done for the "user script isolate".)

* If Peter comes back with a "will not fix", then we can change this CL to
loading the service isolate from a dill file.

[sivachandra, 2017-06-08 00:13:16]

Also, as it stands now, the Python script (which generates the kernel data
array) needs a little bit of cleanup.

[siva, 2017-06-08 01:05:18]

Do you think Peter will have a fix soon for the bug? We may have to go with the
external file initially if it is going to take longer for him.

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

https://codereview.chromium.org/2925203002/diff/1/runtime/bin/main.cc#newcode50
runtime/bin/main.cc:50: DECLARE_FLAG(bool, use_dart_frontend);
This is not needed you can use dfe.UseDartFrontEnd() for this

[ahe, 2017-06-08 08:03:47]

On 2017/06/08 01:05:18, siva wrote:
> Do you think Peter will have a fix soon for the bug? We may have to go with
the
> external file initially if it is going to take longer for him.

I've requested a meeting with sivachandra. It's not clear to me what it is
missing. dart:vmservice_io is already in platform.dill.

[sivachandra, 2017-06-08 16:21:06]

On 2017/06/08 08:03:47, ahe wrote:
> On 2017/06/08 01:05:18, siva wrote:
> > Do you think Peter will have a fix soon for the bug? We may have to go with
> the
> > external file initially if it is going to take longer for him.
> 
> I've requested a meeting with sivachandra. It's not clear to me what it is
> missing. dart:vmservice_io is already in platform.dill.

OK. I will try using platform.dill directly and get back to you. A question
(mostly for myself) is, why is vmservice_io.dill around 1500 bytes larger than
platform.dill.

[sivachandra, 2017-06-08 17:54:32]

On 2017/06/08 16:21:06, sivachandra wrote:
> On 2017/06/08 08:03:47, ahe wrote:
> > On 2017/06/08 01:05:18, siva wrote:
> > > Do you think Peter will have a fix soon for the bug? We may have to go
with
> > the
> > > external file initially if it is going to take longer for him.
> > 
> > I've requested a meeting with sivachandra. It's not clear to me what it is
> > missing. dart:vmservice_io is already in platform.dill.
> 
> OK. I will try using platform.dill directly and get back to you. A question
> (mostly for myself) is, why is vmservice_io.dill around 1500 bytes larger than
> platform.dill.

Loading platform.dill and setting dart:vmservice_io as the root library seems to
be working! I will test this a bit more thoroughly. If it is indeed working
properly, then we do not need to most of this CL!

[I have not answered why vmservice_io.dill is larger than platform.dill.]

[sivachandra, 2017-06-13 01:10:52]

PTAL at patch set 3. The change there follows the approach of loading the
service isolate from a kernel file stored on disk.

[sivachandra, 2017-06-13 01:11:32]

Description was changed from

==========
Load service isolate from the kernel binary.
==========

to

==========
Load service isolate from a .dill file.
==========

[siva, 2017-06-13 02:12:02]

LGTM with some comments.

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

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:96: return NULL;
I think this can also be changed to
{
  kernel_platform_ = ReadScript(platform_binary_filename_);
  return kernel_platform_;
}

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:109: return NULL;
Can this be:

{
  kernel_vmservice_io_ = ReadScript(vmservice_io_binary_filename_);
  return kernel_vmservice_io_;
}

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

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:630: {"--vmservice_io=", ProcessVmServiceIOOption},
Can we derive the location of this file from the executable instead of yet
another command line option?

Maybe we should do the same for --platform too in another CL.

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1069: Dart_ShutdownIsolate();
Why does shutdown isolate need to be called here? the isolate has not started
running right, it has just been created and the vmservice app has been loaded
into it.

[sivachandra, 2017-06-14 19:25:57]

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

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:96: return NULL;
On 2017/06/13 02:12:02, siva wrote:
> I think this can also be changed to
> {
>   kernel_platform_ = ReadScript(platform_binary_filename_);
>   return kernel_platform_;
> }

Doing this in https://codereview.chromium.org/2933203004/.

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:109: return NULL;
On 2017/06/13 02:12:02, siva wrote:
> Can this be:
> 
> {
>   kernel_vmservice_io_ = ReadScript(vmservice_io_binary_filename_);
>   return kernel_vmservice_io_;
> }

Will update this after https://codereview.chromium.org/2933203004/ lands.

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

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:630: {"--vmservice_io=", ProcessVmServiceIOOption},
On 2017/06/13 02:12:02, siva wrote:
> Can we derive the location of this file from the executable instead of yet
> another command line option?
> 
> Maybe we should do the same for --platform too in another CL.

I proposed a way to reduce the number of options here:
https://codereview.chromium.org/2933203004/. A few things which that change
does:

1. Formalizes the file name "platform.dill" in the VM. We can update with an
additional file name "vmservice_io.dill" for the VM service in this CL. When we
move to snapshots, we probably change .dill extension to .snapshot extension.
2. The VM does not deduce or guess the location of these files; It explicitly
expects them in the --kernel-binaries directory. It is upto the build system to
decide what this directory is. Today, it is $build_dir/patched_sdk.

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1069: Dart_ShutdownIsolate();
On 2017/06/13 02:12:02, siva wrote:
> Why does shutdown isolate need to be called here? the isolate has not started
> running right, it has just been created and the vmservice app has been loaded
> into it.

I thought ShutdownIsolate reverses the side effects of CreateIsolate. Since we
already created the isolate at this point, how to effect a reversal? Should it
be done at all?

[sivachandra, 2017-06-21 17:47:56]

Patch set 4 addresses all the comments and rebased over all the dependent CLs
which have now landed. Will land this CL shortly.

[sivachandra, 2017-06-21 19:16:49]

Description was changed from

==========
Load service isolate from a .dill file.
==========

to

==========
Load service isolate from a .dill file.

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b0c2a382ca70bc0f2270e4b8c7ccc76167f256cc
==========

[sivachandra, 2017-06-21 19:16:50]

Committed patchset #4 (id:60001) manually as
b0c2a382ca70bc0f2270e4b8c7ccc76167f256cc (presubmit successful).

[siva, 2017-06-21 20:01:26]

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

https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1069: Dart_ShutdownIsolate();
On 2017/06/14 19:25:57, sivachandra wrote:
> On 2017/06/13 02:12:02, siva wrote:
> > Why does shutdown isolate need to be called here? the isolate has not
started
> > running right, it has just been created and the vmservice app has been
loaded
> > into it.
> 
> I thought ShutdownIsolate reverses the side effects of CreateIsolate. Since we
> already created the isolate at this point, how to effect a reversal? Should it
> be done at all?

You are right I did not see that the isolate was created and an
Dart_EnterScope() had already been done.

So the code would be:
Dart_ExitScope();
Dart_ShutdownIsolate();

[sivachandra, 2017-06-21 20:26:07]

On 2017/06/21 20:01:26, siva wrote:
> https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc
> File runtime/bin/main.cc (right):
> 
>
https://codereview.chromium.org/2925203002/diff/40001/runtime/bin/main.cc#new...
> runtime/bin/main.cc:1069: Dart_ShutdownIsolate();
> On 2017/06/14 19:25:57, sivachandra wrote:
> > On 2017/06/13 02:12:02, siva wrote:
> > > Why does shutdown isolate need to be called here? the isolate has not
> started
> > > running right, it has just been created and the vmservice app has been
> loaded
> > > into it.
> > 
> > I thought ShutdownIsolate reverses the side effects of CreateIsolate. Since
we
> > already created the isolate at this point, how to effect a reversal? Should
it
> > be done at all?
> 
> You are right I did not see that the isolate was created and an
> Dart_EnterScope() had already been done.
> 
> So the code would be:
> Dart_ExitScope();
> Dart_ShutdownIsolate();

Yes; patch set 4 does that now, but unfortunately, I am having to revert this.