VM: [Kernel] Fix bootstraping when Kernel isolate is used.

VM: [Kernel] Fix bootstraping when Kernel isolate is used.

We must bootstrap from Kernel instead of Source when running with --dfe

BUG=
R=asiva@google.com, kustermann@google.com

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

[hausner, 2017-01-24 00:50:54]

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

[hausner, 2017-01-24 00:50:56]

Comments and questions. Siva is also looking at the CL.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/loader.cc#newco...
runtime/bin/loader.cc:648: loader->SendKernelRequest(tag, url);
Is this code path still used? It seems to me that it is not, and if so, it
should be removed.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode861
runtime/bin/main.cc:861: Dart_KernelCompilationResult result =
Dart_CompileToKernel(script_uri);
Calling the front end here only works because it returns a fully linked Kernel
binary, containing the entire core library. Correct?

We want the Dart/Kernel front end to return an incremental Kernel binary
eventually, i.e. a binary that does not contain the core libraries (only
references to the names used in the script). The core libraries will then have
to be loaded from a snapshot again, possibly a new snapshot format which
contains Kernel binary blobs.

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode968
runtime/bin/main.cc:968: Dart_Port port = Dart_ServiceWaitForKernelPort();
Haven't we waited for the kernel isolate to be ready when calling
Dart_CompileToKernel() above?

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode974
runtime/bin/main.cc:974: Loader::InitForSnapshot(script_uri);
Why is this call needed here? This seems to be app snapshot specific. Maybe the
name of the function should change if it does something that is necessary when
otherwise the loader is not invoked?

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode...
runtime/bin/main.cc:1002: }
else {
  // Do the InitForSnapshot() call here?
}

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

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:6010: DART_EXPORT Dart_Port
Dart_ServiceWaitForKernelPort() {
This seems to be dead code now.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:6019: DART_EXPORT Dart_Port Dart_KernelPort() {
ditto.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.h
File runtime/vm/kernel_isolate.h (right):

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.h...
runtime/vm/kernel_isolate.h:30: static bool LoadScript(const String& url, const
String& source);
LoadScript seems to have no implementation (even before your change). Should be
removed.

[siva, 2017-01-24 02:38:01]

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

[siva, 2017-01-24 02:38:02]

lgtm with some comments.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode875
runtime/bin/main.cc:875: case Dart_KernelCompilationStatus_Unknown:
This return code seems to happen only when the kernel isolate itself doesn't
start up. Why not set this up as
*exit_code = kErrorExitCode;


(otherwise exit code ends up being 0 in this case)

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode968
runtime/bin/main.cc:968: Dart_Port port = Dart_ServiceWaitForKernelPort();
On 2017/01/24 00:50:55, hausner wrote:
> Haven't we waited for the kernel isolate to be ready when calling
> Dart_CompileToKernel() above?

Maybe move this up to just before where Dart_CompileToKernel is called
and add the kernel_port as a parameter to Dart_CompileToKernel
instead of having this code inside of Dart_CompileToKernel()

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:37: // Note: this values must match
Dart_KernelCompilationStatus in dart_api.h.
these values

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:59: Map<String, dynamic> toJson();
Sometimes there are blank lines between methods and sometimes not, would nice to
always have a blank line between methods to make it easier to read.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:367: ml.NotifyAll();
Why does it have to do a notifyAll, it appears that we have only one thread
waiting above in SendAndWaitForResponse

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:379: Dart_KernelCompilationResult result_;
The boiler plate DISALLOW_* stuff is missing.

[kustermann, 2017-01-30 12:14:22]

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

[kustermann, 2017-01-30 12:14:29]

LGTM, added some nits

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

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode816
runtime/bin/main.cc:816: // Returns true on success, false on failure.
This comment says it returns true/false, but it seems to be rather returning
some kind of pointer or NULL.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:40: const STATUS_CRASH = 2; // Compiler
crashed.
more types: int?

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:67: }
The fromJson/toJson doesn't seem to be used in this CL. Maybe get rid of them?

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:171: Uri patchedSdk =
final :)

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:182: CompilationResult result;
unused variable

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:187: Future _processLoadRequest(request) async
{
Maybe add a comment describing where the other end of [request] is (i.e. the c++
code sending the requests).

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:216: port.send([-tag, inputFileUrl,
inputFileUrl, null, result.errorString]);
Also add a pointer here where in c++ this is received.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:6031: return false;
The return type is Dart_KernelCompilationResult which is a struct:

+typedef struct {
+  Dart_KernelCompilationStatus status;
+  char* error;
+
+  uint8_t* kernel;
+  intptr_t kernel_size;
+} Dart_KernelCompilationResult;
+

How does this even compile?

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

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:274: class KernelCompilationRequest {
: public StackResource

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:329: free(script_uri_copy);
Why do you strdup() it first, and free it here. Can't you just pass the pointer
and not free it?

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:367: ml.NotifyAll();
On 2017/01/24 02:38:02, siva wrote:
> Why does it have to do a notifyAll, it appears that we have only one thread
> waiting above in SendAndWaitForResponse

Agreed. Notify() should be enough

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:379: Dart_KernelCompilationResult result_;
On 2017/01/24 02:38:02, siva wrote:
> The boiler plate DISALLOW_* stuff is missing.

I think it can be inherited via StackResource.

[Vyacheslav Egorov (Google), 2017-01-30 19:34:14]

Thanks for the review. Landing.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/loader.cc#newco...
runtime/bin/loader.cc:648: loader->SendKernelRequest(tag, url);
On 2017/01/24 00:50:55, hausner wrote:
> Is this code path still used? It seems to me that it is not, and if so, it
> should be removed.

This code path will be used again when we start doing things like deferred
library support. I think we should keep it in for now.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode816
runtime/bin/main.cc:816: // Returns true on success, false on failure.
On 2017/01/30 12:14:23, kustermann wrote:
> This comment says it returns true/false, but it seems to be rather returning
> some kind of pointer or NULL.

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode861
runtime/bin/main.cc:861: Dart_KernelCompilationResult result =
Dart_CompileToKernel(script_uri);
On 2017/01/24 00:50:55, hausner wrote:
> Calling the front end here only works because it returns a fully linked Kernel
> binary, containing the entire core library. Correct?
> 
> We want the Dart/Kernel front end to return an incremental Kernel binary
> eventually, i.e. a binary that does not contain the core libraries (only
> references to the names used in the script). The core libraries will then have
> to be loaded from a snapshot again, possibly a new snapshot format which
> contains Kernel binary blobs.
>  

Yep, your understanding of the current situation is correct. We are slowly
moving towards application and core split.

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode875
runtime/bin/main.cc:875: case Dart_KernelCompilationStatus_Unknown:
On 2017/01/24 02:38:02, siva wrote:
> This return code seems to happen only when the kernel isolate itself doesn't
> start up. Why not set this up as
> *exit_code = kErrorExitCode;
> 
> 
> (otherwise exit code ends up being 0 in this case)

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode968
runtime/bin/main.cc:968: Dart_Port port = Dart_ServiceWaitForKernelPort();
On 2017/01/24 02:38:02, siva wrote:
> On 2017/01/24 00:50:55, hausner wrote:
> > Haven't we waited for the kernel isolate to be ready when calling
> > Dart_CompileToKernel() above?
> 
> Maybe move this up to just before where Dart_CompileToKernel is called
> and add the kernel_port as a parameter to Dart_CompileToKernel
> instead of having this code inside of Dart_CompileToKernel()

Removed this code.

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode974
runtime/bin/main.cc:974: Loader::InitForSnapshot(script_uri);
On 2017/01/24 00:50:55, hausner wrote:
> Why is this call needed here? This seems to be app snapshot specific. Maybe
the
> name of the function should change if it does something that is necessary when
> otherwise the loader is not invoked?

Some parts of core-libraries (e.g. Isolate methods) query loader state to
resolve packages mapping, package relative uris, etc, so we need loader in
initialized state.

I am adding a comment.

https://codereview.chromium.org/2651633002/diff/1/runtime/bin/main.cc#newcode...
runtime/bin/main.cc:1002: }
On 2017/01/24 00:50:55, hausner wrote:
> else {
>   // Do the InitForSnapshot() call here?
> }

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:37: // Note: this values must match
Dart_KernelCompilationStatus in dart_api.h.
On 2017/01/24 02:38:02, siva wrote:
> these values

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:40: const STATUS_CRASH = 2; // Compiler
crashed.
On 2017/01/30 12:14:23, kustermann wrote:
> more types: int?

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:59: Map<String, dynamic> toJson();
On 2017/01/24 02:38:02, siva wrote:
> Sometimes there are blank lines between methods and sometimes not, would nice
to
> always have a blank line between methods to make it easier to read.

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:67: }
On 2017/01/30 12:14:23, kustermann wrote:
> The fromJson/toJson doesn't seem to be used in this CL. Maybe get rid of them?

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:171: Uri patchedSdk =
On 2017/01/30 12:14:24, kustermann wrote:
> final :)

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:182: CompilationResult result;
On 2017/01/30 12:14:23, kustermann wrote:
> unused variable

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:187: Future _processLoadRequest(request) async
{
On 2017/01/30 12:14:23, kustermann wrote:
> Maybe add a comment describing where the other end of [request] is (i.e. the
c++
> code sending the requests).

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/tools/kernel-servic...
runtime/tools/kernel-service.dart:216: port.send([-tag, inputFileUrl,
inputFileUrl, null, result.errorString]);
On 2017/01/30 12:14:23, kustermann wrote:
> Also add a pointer here where in c++ this is received.

Done.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:6010: DART_EXPORT Dart_Port
Dart_ServiceWaitForKernelPort() {
On 2017/01/24 00:50:55, hausner wrote:
> This seems to be dead code now.

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:6019: DART_EXPORT Dart_Port Dart_KernelPort() {
On 2017/01/24 00:50:55, hausner wrote:
> ditto.

Acknowledged.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:6031: return false;
On 2017/01/30 12:14:25, kustermann wrote:
> The return type is Dart_KernelCompilationResult which is a struct:
> 
> +typedef struct {
> +  Dart_KernelCompilationStatus status;
> +  char* error;
> +
> +  uint8_t* kernel;
> +  intptr_t kernel_size;
> +} Dart_KernelCompilationResult;
> +
> 
> How does this even compile?

Done.

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

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:274: class KernelCompilationRequest {
On 2017/01/30 12:14:26, kustermann wrote:
> : public StackResource 

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:329: free(script_uri_copy);
On 2017/01/30 12:14:28, kustermann wrote:
> Why do you strdup() it first, and free it here. Can't you just pass the
pointer
> and not free it?

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:367: ml.NotifyAll();
On 2017/01/30 12:14:26, kustermann wrote:
> On 2017/01/24 02:38:02, siva wrote:
> > Why does it have to do a notifyAll, it appears that we have only one thread
> > waiting above in SendAndWaitForResponse
> 
> Agreed. Notify() should be enough

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.c...
runtime/vm/kernel_isolate.cc:379: Dart_KernelCompilationResult result_;
On 2017/01/30 12:14:28, kustermann wrote:
> On 2017/01/24 02:38:02, siva wrote:
> > The boiler plate DISALLOW_* stuff is missing.
> 
> I think it can be inherited via StackResource.

Done.

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.h
File runtime/vm/kernel_isolate.h (right):

https://codereview.chromium.org/2651633002/diff/1/runtime/vm/kernel_isolate.h...
runtime/vm/kernel_isolate.h:30: static bool LoadScript(const String& url, const
String& source);
On 2017/01/24 00:50:55, hausner wrote:
> LoadScript seems to have no implementation (even before your change). Should
be
> removed.

Done.

[Vyacheslav Egorov (Google), 2017-01-30 19:53:03]

Description was changed from

==========
VM: [Kernel] Fix bootstraping when Kernel isolate is used.

We must bootstrap from Kernel instead of Source when running with --dfe

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

to

==========
VM: [Kernel] Fix bootstraping when Kernel isolate is used.

We must bootstrap from Kernel instead of Source when running with --dfe

BUG=
R=asiva@google.com, kustermann@google.com

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

[Vyacheslav Egorov (Google), 2017-01-30 19:53:05]

Committed patchset #2 (id:20001) manually as
43a0500e37c67184f8cc8abe5afa6ff1525837f3 (presubmit successful).