Add Kernel Isolate

Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages  foo.dart

The front end expects a directory named patched_sdk in the build directory (patched_sdk should be a sibling of the directory that contains the dart executable.)

Warning: using a debug build dart executable is very slow. It takes 2 minutes to run Hello World in a debug build, and about 7 seconds in a release build.

BUG=
R=asiva@google.com

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

[hausner, 2016-12-01 19:34:05]

Description was changed from

==========
Add Kernel Isolate

BUG=
==========

to

==========
Add Kernel Isolate

BUG=
==========

[hausner, 2016-12-01 19:34:05]

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

[hausner, 2016-12-01 19:34:26]

We can discuss details offline.

[Cutch, 2016-12-01 19:47:23]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-12-01 19:47:23]

DBC

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

https://codereview.chromium.org/2483373002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:912: Log::Print("Waiting for Kernel isolate to load.\n");
we shouldn't print to stdout here because it will interfere with the program's
output.

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

https://codereview.chromium.org/2483373002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:3140: DART_EXPORT bool
Dart_IsKernelIsolate(Dart_Isolate isolate);
doc comments

https://codereview.chromium.org/2483373002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:3143: DART_EXPORT Dart_Port Dart_KernelPort();
the API could be factored into something like this:

enum DartServiceIsolateKind {
  kVMServiceIsolate,
  kKernelServiceIsolate,
}

and then the API could take the kind:

bool Dart_ServiceIsolateIsRunning(DartServiceIsolateKind kind);

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.h (right):

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.h:19: class KernelIsolate : public AllStatic {
Lots of this code could be shared between the service isolate and the kernel
isolate. It would be nice to not duplicate code for the KernelIsolate.

[Florian Schneider, 2016-12-01 20:50:43]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2016-12-01 20:50:43]

dbc:

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:315: KernelIsolate::Run();
Please make sure that all code related to this new isolate is not included in
dart_precompiled_runtime. We use the DART_PRECOMPILED_RUNTIME macro to
include/exclude code from it.

[hausner, 2016-12-01 21:00:42]

Patchset #1 (id:1) has been deleted

[hausner, 2016-12-01 22:43:32]

Exclude the Kernel Isolate from precompiled runtime.

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:315: KernelIsolate::Run();
On 2016/12/01 20:50:43, Florian Schneider wrote:
> Please make sure that all code related to this new isolate is not included in
> dart_precompiled_runtime. We use the DART_PRECOMPILED_RUNTIME macro to
> include/exclude code from it.

Done.

[hausner, 2016-12-02 00:32:36]

Description was changed from

==========
Add Kernel Isolate

BUG=
==========

to

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages

Warning: using a debug build dart executable is very slow.

BUG=
==========

[siva, 2016-12-02 00:39:24]

Publishing what I have so far, I will continue with the new version you have
uploaded.

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

https://codereview.chromium.org/2483373002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:915: Dart_ServiceWaitForKernelPort();
If this returns with ILLEGAL_PORT we should return NULL right so that it will be
processed as an error.

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:146: return false;
This false return is weird here as we will continue and start executing the
message handler Run after this even though the result was an error
and not shutdown the isolate.

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:158: return false;
ditto comment here about the false return.

https://codereview.chromium.org/2483373002/diff/20001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:174: return false;
ditto comment here about the false return

[siva, 2016-12-02 20:59:38]

https://codereview.chromium.org/2483373002/diff/40001/runtime/tools/kernel-se...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2483373002/diff/40001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:100: "packages": "/src/d2/sdk/.packages",
These paths here seem to indicate this would work only on mac and in a debug
build.

Can these paths be derived from the path of 'dartk' passed in ?

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:72: KernelIsolate::FinishedExiting();
we seem to grab the lock thrice and do a notifyAll twice here, would it make
sense to have a KernelIsolate::ExitingWithError()

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:84:
ShutdownIsolate(reinterpret_cast<uword>(isolate));
initializing state should be set to false here?

If there is an ExitingWithError that could be used here too.

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:91: ASSERT(KernelIsolate::isolate_ == isolate);
This is an unlocked access, probably ok but maybe
ASSERT(KernelIsolate::IsKernelIsolate(isolate));
would be safer and would remove the need to friend this in KernelIsolate

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.h (right):

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.h:57: };
I agree that this class can be refactored to support both the ServiceIsolate and
KernelIsolate, it may have to morph from an AllStatic call to a regular class.

Maybe do it in a new cl.

[hausner, 2016-12-03 01:29:00]

This addresses most of the comments.
One more round to go.

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

https://codereview.chromium.org/2483373002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:912: Log::Print("Waiting for Kernel isolate to load.\n");
On 2016/12/01 19:47:23, Cutch wrote:
> we shouldn't print to stdout here because it will interfere with the program's
> output.

Done.

https://codereview.chromium.org/2483373002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:915: Dart_ServiceWaitForKernelPort();
On 2016/12/02 00:39:23, siva wrote:
> If this returns with ILLEGAL_PORT we should return NULL right so that it will
be
> processed as an error.

If this return ILLEGAL_PORT, e.g. because there was an error in loading the
front end, then we currently just fall back to the C front end. Maybe that just
silently ignores potential errors in the dart front end. 

Added checks as you proposed.

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

https://codereview.chromium.org/2483373002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:3140: DART_EXPORT bool
Dart_IsKernelIsolate(Dart_Isolate isolate);
On 2016/12/01 19:47:23, Cutch wrote:
> doc comments

Added todo.

https://codereview.chromium.org/2483373002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:3143: DART_EXPORT Dart_Port Dart_KernelPort();
On 2016/12/01 19:47:23, Cutch wrote:
> the API could be factored into something like this:
> 
> enum DartServiceIsolateKind {
>   kVMServiceIsolate,
>   kKernelServiceIsolate,
> }
> 
> and then the API could take the kind:
> 
> bool Dart_ServiceIsolateIsRunning(DartServiceIsolateKind kind);

Good suggestion. I would like to wait with this until I know better how the
Kernel isolate code will work. For this prototype I'd like to avoid changing the
Service Isolate code.

https://codereview.chromium.org/2483373002/diff/40001/runtime/tools/kernel-se...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2483373002/diff/40001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:100: "packages": "/src/d2/sdk/.packages",
On 2016/12/02 20:59:38, siva wrote:
> These paths here seem to indicate this would work only on mac and in a debug
> build.
> 
> Can these paths be derived from the path of 'dartk' passed in ?

Derived them from the dart executable path and from the packageConfig in the
Platform class.

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:72: KernelIsolate::FinishedExiting();
On 2016/12/02 20:59:38, siva wrote:
> we seem to grab the lock thrice and do a notifyAll twice here, would it make
> sense to have a KernelIsolate::ExitingWithError()

Eliminated one state and one call. I don't think the lock grabbing is
prohibitively expensive, this is an error path that leads to the shutdown of the
VM.

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:84:
ShutdownIsolate(reinterpret_cast<uword>(isolate));
On 2016/12/02 20:59:38, siva wrote:
> initializing state should be set to false here?
> 
> If there is an ExitingWithError that could be used here too.

Done.

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:91: ASSERT(KernelIsolate::isolate_ == isolate);
On 2016/12/02 20:59:38, siva wrote:
> This is an unlocked access, probably ok but maybe
> ASSERT(KernelIsolate::IsKernelIsolate(isolate));
> would be safer and would remove the need to friend this in KernelIsolate

Done. The friending is still necessary, for some protected/private KernelIsolate
methods.

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.h (right):

https://codereview.chromium.org/2483373002/diff/40001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.h:57: };
On 2016/12/02 20:59:38, siva wrote:
> I agree that this class can be refactored to support both the ServiceIsolate
and
> KernelIsolate, it may have to morph from an AllStatic call to a regular class.
> 
> Maybe do it in a new cl.

I agree, but would like to address this in a later CL.

[siva, 2016-12-05 22:36:10]

lgtm

https://codereview.chromium.org/2483373002/diff/60001/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2483373002/diff/60001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:646: if (Dart_KernelIsolateIsRunning()) {
You could potentially turn this into

if (use_dart_frontend) {
  if (Dart_KernelIsolateRunning()) {
    loader->SendKernelRequest(tag, url);
  } else {
    return Dart_NewApiError("Cannot load sources as kernel isolate is not
running");
  }

to ensure that we don't run code through the regular loading process when -dfe
is specified on the command line.

Please feel free to ignore this comment if the plan is abort execution when the
kernel isolate fails to start during the initialize process.

[hausner, 2016-12-06 19:03:45]

Description was changed from

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages

Warning: using a debug build dart executable is very slow.

BUG=
==========

to

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages 
foo.dart

Warning: using a debug build dart executable is very slow. It takes 2 minutes to
run Hello World in a debug build, and about 7 seconds in a release build.

BUG=
==========

[hausner, 2016-12-06 19:06:46]

Description was changed from

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages 
foo.dart

Warning: using a debug build dart executable is very slow. It takes 2 minutes to
run Hello World in a debug build, and about 7 seconds in a release build.

BUG=
==========

to

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages 
foo.dart

The front end expects a directory named patched_sdk in the build directory
(patched_sdk should be a sibling of the directory that contains the dart
executable.)

Warning: using a debug build dart executable is very slow. It takes 2 minutes to
run Hello World in a debug build, and about 7 seconds in a release build.

BUG=
==========

[hausner, 2016-12-06 19:10:28]

Thanks for the reviews. This will land soon.

https://codereview.chromium.org/2483373002/diff/60001/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2483373002/diff/60001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:646: if (Dart_KernelIsolateIsRunning()) {
On 2016/12/05 22:36:10, siva wrote:
> You could potentially turn this into
> 
> if (use_dart_frontend) {
>   if (Dart_KernelIsolateRunning()) {
>     loader->SendKernelRequest(tag, url);
>   } else {
>     return Dart_NewApiError("Cannot load sources as kernel isolate is not
> running");
>   }
> 
> to ensure that we don't run code through the regular loading process when -dfe
> is specified on the command line.
> 
> Please feel free to ignore this comment if the plan is abort execution when
the
> kernel isolate fails to start during the initialize process.

This doesn't work because the Kernel isolate code is loaded through this code
path as well. In that case, the KernelIsolate isn't running of course, and we'd
report the error.

[hausner, 2016-12-06 20:23:12]

Description was changed from

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages 
foo.dart

The front end expects a directory named patched_sdk in the build directory
(patched_sdk should be a sibling of the directory that contains the dart
executable.)

Warning: using a debug build dart executable is very slow. It takes 2 minutes to
run Hello World in a debug build, and about 7 seconds in a release build.

BUG=
==========

to

==========
Add Kernel Isolate

To run a script with the Dart->Kernel parser, use the -dfe option:

dart --dfe=runtime/tools/kernel-service.dart  --packages=/src/d2/sdk/.packages 
foo.dart

The front end expects a directory named patched_sdk in the build directory
(patched_sdk should be a sibling of the directory that contains the dart
executable.)

Warning: using a debug build dart executable is very slow. It takes 2 minutes to
run Hello World in a debug build, and about 7 seconds in a release build.

BUG=
R=asiva@google.com

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

[hausner, 2016-12-06 20:23:13]

Committed patchset #5 (id:100001) manually as
c9399fc5571584336eeda46e5aba4b1d5a87bd0e (presubmit successful).