Option to output precompiled instructions as a blob for use with mmap instead of assembly for use i…

Option to output precompiled instructions as a blob for use with mmap instead of assembly for use in a shared library.

BUG=http://dartbug.com/26199
R=fschneider@google.com

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

[rmacnak, 2016-04-25 19:54:46]

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

[Florian Schneider, 2016-04-25 21:40:27]

Lgtm!

Please make sure to test all the command-line options for gen_snapshot as well
before landing.

I guess eventually we should switch to using gen_snapshot only - so that we can
avoid duplication between main.cc and gen_snapshot.cc.

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/gen_snapsh...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/gen_snapsh...
runtime/bin/gen_snapshot.cc:327: bool precompiled_as_blobs =
(instructions_blob_filename != NULL) ||
This is a bit confusing: Shouldn't this be && instead of ||? i.e. only if both
blob-options are present should precompiled_as_blobs be true?

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/main.cc
File runtime/bin/main.cc (left):

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/main.cc#ol...
runtime/bin/main.cc:1613: LoadLibrarySymbol(precompiled_snapshot_directory,
It seems that we load the library twice by accident? i.e. each call to
LoadLibrarySymbol invokes Extensions::LoadExtensionLibrary. I think we shoud
avoid that.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (left):

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:6107: Dart_Handle state =
Api::CheckAndFinalizePendingClasses(T);
Is this because this is already done in Dart_Precompile?

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h
File runtime/vm/snapshot.h (right):

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:909: void Write();
Maybe add virtual here too for documentation purposes.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:910: intptr_t binary_size() { return binary_size_; }
virtual

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:943: void Write();
virtual

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:944: intptr_t binary_size() {
virtual

[rmacnak, 2016-04-25 23:16:07]

Patched this into a Flutter tree and I was able to build for iOS only make the
--instructions-snapshot -> --assembly change.

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/gen_snapsh...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/gen_snapsh...
runtime/bin/gen_snapshot.cc:327: bool precompiled_as_blobs =
(instructions_blob_filename != NULL) ||
On 2016/04/25 21:40:26, Florian Schneider wrote:
> This is a bit confusing: Shouldn't this be && instead of ||? i.e. only if both
> blob-options are present should precompiled_as_blobs be true?

This is to make all of these errors:
 
--assembly --instructions_blob
--assembly --instructions_blob --rodata_blob
--assembly --rodata_blob

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/main.cc
File runtime/bin/main.cc (left):

https://codereview.chromium.org/1915853004/diff/100001/runtime/bin/main.cc#ol...
runtime/bin/main.cc:1613: LoadLibrarySymbol(precompiled_snapshot_directory,
On 2016/04/25 21:40:26, Florian Schneider wrote:
> It seems that we load the library twice by accident? i.e. each call to
> LoadLibrarySymbol invokes Extensions::LoadExtensionLibrary. I think we shoud
> avoid that.

Yes, there is currently a redundant dlopen.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (left):

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:6107: Dart_Handle state =
Api::CheckAndFinalizePendingClasses(T);
On 2016/04/25 21:40:26, Florian Schneider wrote:
> Is this because this is already done in Dart_Precompile?

Right. Leftover from when the function was originally copied from
Dart_CreateSnapshot.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h
File runtime/vm/snapshot.h (right):

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:909: void Write();
On 2016/04/25 21:40:27, Florian Schneider wrote:
> Maybe add virtual here too for documentation purposes.

Done.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:910: intptr_t binary_size() { return binary_size_; }
On 2016/04/25 21:40:27, Florian Schneider wrote:
> virtual

Done.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:943: void Write();
On 2016/04/25 21:40:26, Florian Schneider wrote:
> virtual

Done.

https://codereview.chromium.org/1915853004/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:944: intptr_t binary_size() {
On 2016/04/25 21:40:27, Florian Schneider wrote:
> virtual

Done.

[rmacnak, 2016-04-25 23:19:38]

Description was changed from

==========
Option to output precompiled instructions as a blob for use with mmap instead of
assembly for use in a shared library.

BUG=http://dartbug.com/26199
==========

to

==========
Option to output precompiled instructions as a blob for use with mmap instead of
assembly for use in a shared library.

BUG=http://dartbug.com/26199
R=fschneider@google.com

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

[rmacnak, 2016-04-25 23:19:40]

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