Toward precompiled snapshots.

Toward precompiled snapshots.

Gets as far as encountering null Instructions in the first Dart_Invoke after loading the snapshot.

R=asiva@google.com

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

[rmacnak, 2015-08-27 00:22:28]

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

[rmacnak, 2015-08-27 00:22:29]

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

https://codereview.chromium.org/1318803002/diff/120001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1066: if (has_precompiled) {
For now, load everything from files. The embedder is of course free to
statically link these.

[siva, 2015-09-01 20:58:49]

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

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:303: has_precompile = true;
generate script snapshot and precompile can not be specified at the same time
right?

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1067: const char* kPrecompiledLibName = "libprecompiled.so";
This name seems specific to linux, maybe it should move out to a target specific
file.

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1116: DartUtils::CloseFile(file);
The above code of reading a snapshot file is repeated multiple times, maybe you
could have a helper
ReadSnapshotFile(const char* name, char* buffer, intptr_t* len);

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1225: instructions_size);
You should extract these names as constants and use them in both the
has_precompile and has_precompiled case.
e.g:

i.e extract the stuff you have above
const char* kPrecompiledSymName = "kInstructionsSnapshot";
const char* kPrecompiledVmIsolateName = "precompiled.vmisolate";
const char* kPrecompiledIsolateName = "precompiled.isolate"

and make them available globally in this file.
static const char* kPrecompiledVmIsolateSnapshot = "...";
static const char* kPrecompiledIsolateSnapshot = "....";
static const char* kPrecompiledInstructions = "...";

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/native_api_...
File runtime/vm/native_api_impl.cc (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/native_api_...
runtime/vm/native_api_impl.cc:134: ASSERT(FLAG_load_deferred_eagerly);
instead of asserting like this why not do what we do for
generate_script_snapshots in main.cc

  if (generate_script_snapshot) {
    vm_options.AddArgument("--load_deferred_eagerly");
  }

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/native_api_...
runtime/vm/native_api_impl.cc:165: *vm_isolate_snapshot_size =
writer.VmIsolateSnapshotSize();
if vm_isoolate_snapshot_buffer is NULL this could lead to a crash as you allow
vm_isolate_snapshot_size to be NULL

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object....
runtime/vm/raw_object.h:1002: // If return_address_info_ is a Smi, it is the
offset to the prologue.
The comment here says 'If return_address_info_' which return_address_info_ is it
referring to?

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:37: }
Instead of READ_POINTERS why not call it READ_OBJECT_FIELDS

You could also make the parameters of this macro be
#define READ_OBJECT_FIELDS(from, to, as_reference)
that way it would be applicable even for those cases where to is not raw->to()

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:1354: reader->NewStackmap(length));
This could also be created as NEW_OBJECT_WITH_LEN ?

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:1409: (*reader->StringHandle()) ^=
reader->ReadObjectImpl(kAsReference);
The write side is writing as kAsInlinedObject and the read side is reading as
kAsReference

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:1631: }
Maybe move this down to the end as discussed offline.

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:2459: if (kind() == Snapshot::kMessage) {
|| kind() == Snapshot::kScript)

[rmacnak, 2015-09-01 23:43:48]

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

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:303: has_precompile = true;
On 2015/09/01 20:58:48, siva wrote:
> generate script snapshot and precompile can not be specified at the same time
> right?

True. Added the opposite check as for creating a script snapshot that we are not
running from a snapshot.

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1067: const char* kPrecompiledLibName = "libprecompiled.so";
On 2015/09/01 20:58:48, siva wrote:
> This name seems specific to linux, maybe it should move out to a target
specific
> file.

Done.

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1116: DartUtils::CloseFile(file);
On 2015/09/01 20:58:49, siva wrote:
> The above code of reading a snapshot file is repeated multiple times, maybe
you
> could have a helper
> ReadSnapshotFile(const char* name, char* buffer, intptr_t* len);

Done.

https://codereview.chromium.org/1318803002/diff/140001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1225: instructions_size);
On 2015/09/01 20:58:48, siva wrote:
> You should extract these names as constants and use them in both the
> has_precompile and has_precompiled case.
> e.g:
> 
> i.e extract the stuff you have above
> const char* kPrecompiledSymName = "kInstructionsSnapshot";
> const char* kPrecompiledVmIsolateName = "precompiled.vmisolate";
> const char* kPrecompiledIsolateName = "precompiled.isolate"
> 
> and make them available globally in this file.
> static const char* kPrecompiledVmIsolateSnapshot = "...";
> static const char* kPrecompiledIsolateSnapshot = "....";
> static const char* kPrecompiledInstructions = "...";

Done.

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/native_api_...
File runtime/vm/native_api_impl.cc (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/native_api_...
runtime/vm/native_api_impl.cc:134: ASSERT(FLAG_load_deferred_eagerly);
On 2015/09/01 20:58:49, siva wrote:
> instead of asserting like this why not do what we do for
> generate_script_snapshots in main.cc
> 
>   if (generate_script_snapshot) {
>     vm_options.AddArgument("--load_deferred_eagerly");
>   }

That's in the embedder before loading. Here it is too late.

When the embedder enabled --precompile it caused --load_deferred_eagerly to also
be enabled.

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/native_api_...
runtime/vm/native_api_impl.cc:165: *vm_isolate_snapshot_size =
writer.VmIsolateSnapshotSize();
On 2015/09/01 20:58:49, siva wrote:
> if vm_isoolate_snapshot_buffer is NULL this could lead to a crash as you allow
> vm_isolate_snapshot_size to be NULL

Now requiring vm_isolate_snapshot_buffer to be non-NULL.

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object....
runtime/vm/raw_object.h:1002: // If return_address_info_ is a Smi, it is the
offset to the prologue.
On 2015/09/01 20:58:49, siva wrote:
> The comment here says 'If return_address_info_' which return_address_info_ is
it
> referring to?

What choice is there other than return_address_metadata_?

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:37: }
On 2015/09/01 20:58:49, siva wrote:
> Instead of READ_POINTERS why not call it READ_OBJECT_FIELDS

Done.

> You could also make the parameters of this macro be
> #define READ_OBJECT_FIELDS(from, to, as_reference)
> that way it would be applicable even for those cases where to is not raw->to()

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:1409: (*reader->StringHandle()) ^=
reader->ReadObjectImpl(kAsReference);
On 2015/09/01 20:58:49, siva wrote:
> The write side is writing as kAsInlinedObject and the read side is reading as
> kAsReference

Done.

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:1631: }
On 2015/09/01 20:58:49, siva wrote:
> Maybe move this down to the end as discussed offline.

Done.

https://codereview.chromium.org/1318803002/diff/140001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:2459: if (kind() == Snapshot::kMessage) {
On 2015/09/01 20:58:49, siva wrote:
> || kind() == Snapshot::kScript)

Done.

[siva, 2015-09-02 21:16:37]

lgtm

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

https://codereview.chromium.org/1318803002/diff/240001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:448: { "--precompiled", ProcessPrecompiledOption },
maybe we should call these
--gen-precompiled-snapshot
--run-precompiled-snapshot

in order to not confuse with the VM options --precompile which is used in both
of the above runs.

https://codereview.chromium.org/1318803002/diff/240001/runtime/include/dart_n...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/1318803002/diff/240001/runtime/include/dart_n...
runtime/include/dart_native_api.h:169: intptr_t* instructions_snapshot_size);
I think Dart_Precompile and Dart_CreatePrecompiledSnapshot should probably move
to dart_api.h

[rmacnak, 2015-09-02 22:58:50]

https://chromiumcodereview.appspot.com/1318803002/diff/240001/runtime/bin/mai...
File runtime/bin/main.cc (right):

https://chromiumcodereview.appspot.com/1318803002/diff/240001/runtime/bin/mai...
runtime/bin/main.cc:448: { "--precompiled", ProcessPrecompiledOption },
On 2015/09/02 21:16:37, siva wrote:
> maybe we should call these
> --gen-precompiled-snapshot
> --run-precompiled-snapshot
> 
> in order to not confuse with the VM options --precompile which is used in both
> of the above runs.

Done.

https://chromiumcodereview.appspot.com/1318803002/diff/240001/runtime/include...
File runtime/include/dart_native_api.h (right):

https://chromiumcodereview.appspot.com/1318803002/diff/240001/runtime/include...
runtime/include/dart_native_api.h:169: intptr_t* instructions_snapshot_size);
On 2015/09/02 21:16:37, siva wrote:
> I think Dart_Precompile and Dart_CreatePrecompiledSnapshot should probably
move
> to dart_api.h

Moved.

[rmacnak, 2015-09-02 23:15:08]

Committed patchset #14 (id:260001) manually as
0ed4d44a996f595bc2a602ccf692f0c90a34e03a (presubmit successful).