Use a single file for app snapshots.

Use a single file for app snapshots.

For blobs, put all four pieces into a single file with a header describing their offsets.
For dynamic libraries, move the vm isolate and isolate pieces into the dynamic library as additional read-only sections.

Automatically detect if the script argument is an app snapshot and initialize the VM appropriately, similar to automatic detection of script snapshots.

R=asiva@google.com

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

[rmacnak, 2016-10-11 23:24:55]

Description was changed from

==========
.


.


.


.


.

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

to

==========
Use a single file for app snapshots.

For blobs, put all four pieces into a single file with a header describing their
offsets.
For dynamic libraries, move the vm isolate and isolate pieces into the dynamic
library as additional read-only sections.

Automatically detect if the script argument is an app snapshot and initialize
the VM appropriately, similar to automatic detection of script snapshots.
==========

[rmacnak, 2016-10-11 23:25:09]

Description was changed from

==========
Use a single file for app snapshots.

For blobs, put all four pieces into a single file with a header describing their
offsets.
For dynamic libraries, move the vm isolate and isolate pieces into the dynamic
library as additional read-only sections.

Automatically detect if the script argument is an app snapshot and initialize
the VM appropriately, similar to automatic detection of script snapshots.
==========

to

==========
Use a single file for app snapshots.

For blobs, put all four pieces into a single file with a header describing their
offsets.
For dynamic libraries, move the vm isolate and isolate pieces into the dynamic
library as additional read-only sections.

Automatically detect if the script argument is an app snapshot and initialize
the VM appropriately, similar to automatic detection of script snapshots.
==========

[rmacnak, 2016-10-12 00:22:00]

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

[siva, 2016-10-12 20:37:34]

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/file.h
File runtime/bin/file.h (right):

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/file.h#newc...
runtime/bin/file.h:92: void* MapReadExecute(int64_t position, int64_t length);
Instead of having two entry points would it make sense to have just one

enum MapType {
  kReadOnly = 0,
  kReadExecute = 1,
  kReadWrite = 2,
};
void* Map(int64_t position, int64_t length, MapType type);

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

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1240: ASSERT(file->Position() == kAppSnapshotHeaderSize);
Instead of these individual reads of the magic number and various sizes why not
do a single read of kAppSnapshotHeaderSize into a buffer that could then be used
to compute the magic number and the various sizes.

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1281: 
Can the order of rodata be swapped with instructions then a single
mmap(PROT_READ) call could be done for the 3 read only sections and the pointers
vmisolate_buffer, isolate_buffer and rodata_buffer could be updated to point to
the addresses into the value returned by that mmap call.

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1282: return true;
Missing file->Release() for the success case?
On Linux at least it is ok to class the fd after an mmap is done not sure if
there are issues on MacOS or Windows.

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1291: void* library =
Extensions::LoadExtensionLibrary(script_name);
Can you make sure that this will work for any name on both linux/macos

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

https://codereview.chromium.org/2405393002/diff/40001/runtime/include/dart_ap...
runtime/include/dart_api.h:3119: intptr_t* assembly_size);
I hope dartium will not be effected by this API change, I assume Flutter will
break and needs to be fixed.

https://codereview.chromium.org/2405393002/diff/40001/tools/testing/dart/runt...
File tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/2405393002/diff/40001/tools/testing/dart/runt...
tools/testing/dart/runtime_configuration.dart:286:
args.add("${artifact.filename}/libprecompiled.dylib");
if any name for the dynamic libraries work then we won't need this
differentiation right.

[rmacnak, 2016-10-13 01:08:16]

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/file.h
File runtime/bin/file.h (right):

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/file.h#newc...
runtime/bin/file.h:92: void* MapReadExecute(int64_t position, int64_t length);
On 2016/10/12 20:37:33, siva wrote:
> Instead of having two entry points would it make sense to have just one
> 
> enum MapType {
>   kReadOnly = 0,
>   kReadExecute = 1,
>   kReadWrite = 2,
> };
> void* Map(int64_t position, int64_t length, MapType type);

Done.

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

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1240: ASSERT(file->Position() == kAppSnapshotHeaderSize);
On 2016/10/12 20:37:33, siva wrote:
> Instead of these individual reads of the magic number and various sizes why
not
> do a single read of kAppSnapshotHeaderSize into a buffer that could then be
used
> to compute the magic number and the various sizes.

Done.

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1281: 
On 2016/10/12 20:37:33, siva wrote:
> Can the order of rodata be swapped with instructions then a single
> mmap(PROT_READ) call could be done for the 3 read only sections and the
pointers
> vmisolate_buffer, isolate_buffer and rodata_buffer could be updated to point
to
> the addresses into the value returned by that mmap call.

Done.

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1282: return true;
On 2016/10/12 20:37:33, siva wrote:
> Missing file->Release() for the success case?
> On Linux at least it is ok to class the fd after an mmap is done not sure if
> there are issues on MacOS or Windows.

Done.

https://codereview.chromium.org/2405393002/diff/40001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1291: void* library =
Extensions::LoadExtensionLibrary(script_name);
On 2016/10/12 20:37:33, siva wrote:
> Can you make sure that this will work for any name on both linux/macos

Done, using the same for both platforms in the test harness now.

[siva, 2016-10-13 01:59:00]

LGTM with comments.

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
File runtime/bin/file_android.cc (right):

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
runtime/bin/file_android.cc:90: break;
default:
  return NULL;
Also in other host files.

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
File runtime/bin/file_fuchsia.cc (right):

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
runtime/bin/file_fuchsia.cc:88: 
Looks like this was not changed to the new File::Map() case.

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
File runtime/bin/file_win.cc (right):

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
runtime/bin/file_win.cc:86: }
Ditto comment about this not being changed for the new File::Map case.

[rmacnak, 2016-10-13 21:17:34]

Also updated gen_snapshot not to emit empty files for vmisolate and isolate when
generating assembly.

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
File runtime/bin/file_android.cc (right):

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
runtime/bin/file_android.cc:90: break;
On 2016/10/13 01:59:00, siva wrote:
> default:
>   return NULL;
> Also in other host files.

Done.

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
File runtime/bin/file_fuchsia.cc (right):

https://chromiumcodereview.appspot.com/2405393002/diff/80001/runtime/bin/file...
runtime/bin/file_fuchsia.cc:88: 
On 2016/10/13 01:59:00, siva wrote:
> Looks like this was not changed to the new File::Map() case.

Done.

[rmacnak, 2016-10-13 21:34:00]

Description was changed from

==========
Use a single file for app snapshots.

For blobs, put all four pieces into a single file with a header describing their
offsets.
For dynamic libraries, move the vm isolate and isolate pieces into the dynamic
library as additional read-only sections.

Automatically detect if the script argument is an app snapshot and initialize
the VM appropriately, similar to automatic detection of script snapshots.
==========

to

==========
Use a single file for app snapshots.

For blobs, put all four pieces into a single file with a header describing their
offsets.
For dynamic libraries, move the vm isolate and isolate pieces into the dynamic
library as additional read-only sections.

Automatically detect if the script argument is an app snapshot and initialize
the VM appropriately, similar to automatic detection of script snapshots.

R=asiva@google.com

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

[rmacnak, 2016-10-13 21:34:02]

Committed patchset #7 (id:120001) manually as
4f8b16df3b3ce9654af03368344706efec247369 (presubmit successful).