Create an app snapshot of the Dart front end.

Create an app snapshot of the Dart front end.

R=asiva@google.com, hausner@google.com, zra@google.com

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

[rmacnak, 2017-01-25 23:59:03]

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

[rmacnak, 2017-01-25 23:59:03]

A first step that does not embed the snapshot in the 'dart' binary.

+ ./tools/build.py -mrelease runtime
+ ./out/ReleaseX64/dart --dfe=runtime/tools/kernel-service.dart
/usr/local/google/home/rmacnak/hello.dart
Hello, world!

real	0m6.350s
user	0m8.777s
sys	0m0.774s

+ ./out/ReleaseX64/dart --dfe=out/ReleaseX64/kernel-service.dart.snapshot
/usr/local/google/home/rmacnak/hello.dart
Hello, world!

real	0m2.558s
user	0m3.090s
sys	0m0.340s

[hausner, 2017-01-26 01:13:37]

LGTM as far as I understand the gn stuff.

The resulting runtime performance gain is fantastic!

[siva, 2017-01-26 01:16:01]

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

[siva, 2017-01-26 01:16:02]

+zra to reviewer list

[siva, 2017-01-26 17:07:08]

lgtm

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

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/main.cc#newcode...
runtime/bin/main.cc:1027: // TODO(rmacnak): We are leaking the snapshot when the
isolate shuts down.
When you say leaked do you mean it is not unmapped?

If two spawnURI isolates share the same script_uri we will be mapping that file
twice, I guess that is ok.

[rmacnak, 2017-01-26 18:07:14]

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

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/main.cc#newcode...
runtime/bin/main.cc:1027: // TODO(rmacnak): We are leaking the snapshot when the
isolate shuts down.
On 2017/01/26 17:07:08, siva wrote:
> When you say leaked do you mean it is not unmapped?
> 
> If two spawnURI isolates share the same script_uri we will be mapping that
file
> twice, I guess that is ok.

Right, it is not unmapped. If a second spawnURI isolate comes in with the same
script uri, we'll consume more virtual memory but no more physical memory.

[zra, 2017-01-26 20:55:00]

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/BUILD.gn
File runtime/bin/BUILD.gn (right):

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/BUILD.gn#newcod...
runtime/bin/BUILD.gn:492: action("generate_dfe_snapshot") {
This should try to use the "application_snapshot" template here:

https://github.com/dart-lang/sdk/blob/master/utils/application_snapshot.gni#L9

If it's not a perfect fit, feel free to modify the template.

I also wonder if runtime/tools/kernel-service.dart and this rule should be moved
to //utils or //pkg to be closer to other things that we make app snapshots for.
Like, //pkg/kernel-service/BUILD.gn, and
//pkg/kernel-service/kernel-service.dart.

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

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/main.cc#newcode812
runtime/bin/main.cc:812: static const int64_t kAppSnapshotHeaderSize = 5 *
sizeof(int64_t);  // NOLINT
is sizeof(int64_t) not already a constant in platform/globals.h or vm/globals.h?

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/main.cc#newcode817
runtime/bin/main.cc:817: static bool ReadAppSnapshotBlobs(const char*
script_name,
Should these snapshot reading functions be pulled out of main.cc into a separate
.cc file? They're starting to take up a lot of main.cc.

https://codereview.chromium.org/2655173002/diff/20001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/2655173002/diff/20001/runtime/bin/bin.gypi#ne...
runtime/bin/bin.gypi:1024: '../tools/create_snapshot_cc.py',
Also undo rename here.

https://codereview.chromium.org/2655173002/diff/20001/runtime/bin/bin.gypi#ne...
runtime/bin/bin.gypi:1033: 'tools/create_snapshot_cc.py',
ditto.

[Siggi Cherem (dart-lang), 2017-01-26 22:38:12]

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

[Siggi Cherem (dart-lang), 2017-01-26 22:38:13]

very exciting to see this Ryan! Just a small drive-by comment below.

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/BUILD.gn
File runtime/bin/BUILD.gn (right):

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/BUILD.gn#newcod...
runtime/bin/BUILD.gn:492: action("generate_dfe_snapshot") {
On 2017/01/26 20:55:00, zra wrote:
> This should try to use the "application_snapshot" template here:
> 
> https://github.com/dart-lang/sdk/blob/master/utils/application_snapshot.gni#L9
> 
> If it's not a perfect fit, feel free to modify the template.
> 
> I also wonder if runtime/tools/kernel-service.dart and this rule should be
moved
> to //utils or //pkg to be closer to other things that we make app snapshots
for.
> Like, //pkg/kernel-service/BUILD.gn, and
> //pkg/kernel-service/kernel-service.dart.

DBC - I like the idea of moving this. Two small related suggestions:

 - consider renaming this script file so we follow the naming conventions we
have everywhere else (remove/replace the "-").

 - could this go under pkg/front_end/bin or pkg/front_end/tool?

I have a CL in progress to update kernel-service.dart to go through
pkg/front_end API (which today wraps dartk but in the future can use fasta). I'm
also fine waiting to do all those changes separately after this lands.

[rmacnak, 2017-01-26 23:48:45]

PTAL.

With the new paths, the commands are
./out/ReleaseX64/dart --dfe=utils/kernel-service/kernel-service.dart
~/hello.dart
./out/ReleaseX64/dart --dfe=out/ReleaseX64/gen/kernel-service.dart.snapshot
~/hello.dart

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/BUILD.gn
File runtime/bin/BUILD.gn (right):

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/BUILD.gn#newcod...
runtime/bin/BUILD.gn:492: action("generate_dfe_snapshot") {
On 2017/01/26 20:55:00, zra wrote:
> This should try to use the "application_snapshot" template here:
> 
> https://github.com/dart-lang/sdk/blob/master/utils/application_snapshot.gni#L9
> 
> If it's not a perfect fit, feel free to modify the template.
> 
> I also wonder if runtime/tools/kernel-service.dart and this rule should be
moved
> to //utils or //pkg to be closer to other things that we make app snapshots
for.
> Like, //pkg/kernel-service/BUILD.gn, and
> //pkg/kernel-service/kernel-service.dart.

Change to application_snapshot template and moved to //utils/kernel-service.
Kept with kernel-service rather than kernel_service to avoid changing some names
inside the VM and consistency with the vm-service. Other can move it further
later if they feel it's important.

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

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/main.cc#newcode812
runtime/bin/main.cc:812: static const int64_t kAppSnapshotHeaderSize = 5 *
sizeof(int64_t);  // NOLINT
On 2017/01/26 20:55:00, zra wrote:
> is sizeof(int64_t) not already a constant in platform/globals.h or
vm/globals.h?

Didn't know about these. Added and used kInt64Size.

https://codereview.chromium.org/2655173002/diff/1/runtime/bin/main.cc#newcode817
runtime/bin/main.cc:817: static bool ReadAppSnapshotBlobs(const char*
script_name,
On 2017/01/26 20:55:00, zra wrote:
> Should these snapshot reading functions be pulled out of main.cc into a
separate
> .cc file? They're starting to take up a lot of main.cc.

Good point, I'll factor this out in a future CL.

https://codereview.chromium.org/2655173002/diff/20001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/2655173002/diff/20001/runtime/bin/bin.gypi#ne...
runtime/bin/bin.gypi:1024: '../tools/create_snapshot_cc.py',
On 2017/01/26 20:55:00, zra wrote:
> Also undo rename here.

Done.

https://codereview.chromium.org/2655173002/diff/20001/runtime/bin/bin.gypi#ne...
runtime/bin/bin.gypi:1033: 'tools/create_snapshot_cc.py',
On 2017/01/26 20:55:00, zra wrote:
> ditto.

Done.

[zra, 2017-01-27 00:04:24]

lgtm

[rmacnak, 2017-01-27 00:29:05]

Description was changed from

==========
Create an app snapshot of the Dart front end.
==========

to

==========
Create an app snapshot of the Dart front end.

R=asiva@google.com, hausner@google.com, zra@google.com

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

[rmacnak, 2017-01-27 00:29:06]

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