Support external startup data in V8.

Support external startup data in V8.

If the embedder chooses, the 'natives' (library sources) and the
precompiled startup blob can be written to files during the build
process and handed over to V8 at startup. The main purpose would be
to reduce the size of the compiled binary for space constrained
platforms.

The build-time option is off by default. Nothing should change if
it's not enabled.

BUG=
R=bmeurer@chromium.org, jochen@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21646

[jochen (gone - plz use gerrit), 2014-05-23 11:44:44]

https://codereview.chromium.org/293993021/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/293993021/diff/1/include/v8.h#newcode4668
include/v8.h:4668: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
embedders won't see this #define, the api should always be present

https://codereview.chromium.org/293993021/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/293993021/diff/1/src/api.cc#newcode350
src/api.cc:350: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
only #ifdef the function body (and maybe add an CHECK(false) in the #else part

https://codereview.chromium.org/293993021/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/293993021/diff/1/src/d8.cc#newcode1320
src/d8.cc:1320: else if (strncmp(argv[i], "--natives_blob=", 15) == 0) {
using - is more consistent with the other options (as opposed to _)

https://codereview.chromium.org/293993021/diff/1/src/d8.cc#newcode1528
src/d8.cc:1528: };
nit. DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/293993021/diff/1/src/mksnapshot.cc
File src/mksnapshot.cc (right):

https://codereview.chromium.org/293993021/diff/1/src/mksnapshot.cc#newcode316
src/mksnapshot.cc:316: void SetSnapshotFromFile(StartupData* data) {
ASSERT(false); }
i guess this should go into snapshot-empty.cc?

https://codereview.chromium.org/293993021/diff/1/src/snapshot-source-sink.h
File src/snapshot-source-sink.h (right):

https://codereview.chromium.org/293993021/diff/1/src/snapshot-source-sink.h#n...
src/snapshot-source-sink.h:22: SnapshotByteSource(const byte* array, int
length);
please add a dtor

https://codereview.chromium.org/293993021/diff/1/src/snapshot-source-sink.h#n...
src/snapshot-source-sink.h:60: };
disallow copy and assign

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp#newcode624
tools/gyp/v8.gyp:624: '../../src/snapshot-source-sink.cc',
can you do those changes (adding/removing files) in BUILD.gn as well? It doesn't
have the snapshot generation logic or anything beyond that yet, so just adding
the files to the list will be enough.

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp#newcode1136
tools/gyp/v8.gyp:1136: '<(SHARED_INTERMEDIATE_DIR)/libraries.bin',
I think we want those in <(PRODUCT_DIR)

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp#newcode1143
tools/gyp/v8.gyp:1143: 'bash',
there's no bash on windows. gyp has some magic to replace cat with type on
windows, however, i'm not sure whether that can cope with binary files, so you
may want to write a small python script for that.

[vogelheim, 2014-05-26 12:36:03]

https://codereview.chromium.org/293993021/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/293993021/diff/1/include/v8.h#newcode4668
include/v8.h:4668: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/05/23 11:44:44, jochen wrote:
> embedders won't see this #define, the api should always be present

Done.

https://codereview.chromium.org/293993021/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/293993021/diff/1/src/api.cc#newcode350
src/api.cc:350: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/05/23 11:44:44, jochen wrote:
> only #ifdef the function body (and maybe add an CHECK(false) in the #else part

Done.

https://codereview.chromium.org/293993021/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/293993021/diff/1/src/d8.cc#newcode1320
src/d8.cc:1320: else if (strncmp(argv[i], "--natives_blob=", 15) == 0) {
On 2014/05/23 11:44:44, jochen wrote:
> using - is more consistent with the other options (as opposed to _)

Done.

https://codereview.chromium.org/293993021/diff/1/src/d8.cc#newcode1528
src/d8.cc:1528: };
On 2014/05/23 11:44:44, jochen wrote:
> nit. DISALLOW_COPY_AND_ASSIGN

d8.cc seems to avoid including internal v8 headers, so I implemented this
without the macro from src/macros.h. Not sure if that's the best way to do it?

https://codereview.chromium.org/293993021/diff/1/src/mksnapshot.cc
File src/mksnapshot.cc (right):

https://codereview.chromium.org/293993021/diff/1/src/mksnapshot.cc#newcode316
src/mksnapshot.cc:316: void SetSnapshotFromFile(StartupData* data) {
ASSERT(false); }
On 2014/05/23 11:44:44, jochen wrote:
> i guess this should go into snapshot-empty.cc?

Done.

Actually, to snapshot-common.cc, since that always gets used when
snapshot-external doesn't.

https://codereview.chromium.org/293993021/diff/1/src/snapshot-source-sink.h
File src/snapshot-source-sink.h (right):

https://codereview.chromium.org/293993021/diff/1/src/snapshot-source-sink.h#n...
src/snapshot-source-sink.h:22: SnapshotByteSource(const byte* array, int
length);
On 2014/05/23 11:44:44, jochen wrote:
> please add a dtor

Done.

https://codereview.chromium.org/293993021/diff/1/src/snapshot-source-sink.h#n...
src/snapshot-source-sink.h:60: };
On 2014/05/23 11:44:44, jochen wrote:
> disallow copy and assign

Done.

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp#newcode624
tools/gyp/v8.gyp:624: '../../src/snapshot-source-sink.cc',
On 2014/05/23 11:44:44, jochen wrote:
> can you do those changes (adding/removing files) in BUILD.gn as well? It
doesn't
> have the snapshot generation logic or anything beyond that yet, so just adding
> the files to the list will be enough.

Done.

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp#newcode1136
tools/gyp/v8.gyp:1136: '<(SHARED_INTERMEDIATE_DIR)/libraries.bin',
On 2014/05/23 11:44:44, jochen wrote:
> I think we want those in <(PRODUCT_DIR)

Done, but only for those two files that are meant to be used outside the v8
build process, snapshot_blob.bin and natives_blob.bin. libraries.bin (which is
an input to natives_blob.bin) remains in SHARED_INTERMEDIATE_DIR.

https://codereview.chromium.org/293993021/diff/1/tools/gyp/v8.gyp#newcode1143
tools/gyp/v8.gyp:1143: 'bash',
On 2014/05/23 11:44:44, jochen wrote:
> there's no bash on windows. gyp has some magic to replace cat with type on
> windows, however, i'm not sure whether that can cope with binary files, so you
> may want to write a small python script for that.

Hmm... I got the idea from code search:
https://code.google.com/p/chromium/codesearch#search/&q=bash%20file:%5C.gyp$&...

Are you sure a separate script is needed?

[jochen (gone - plz use gerrit), 2014-05-26 14:34:49]

lgtm

Benedikt, since Sven is OOO, can you have a look as well?

https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc
File src/natives-external.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc#...
src/natives-external.cc:1: // Copyright 2011 the V8 project authors. All rights
reserved.
nit 2014

https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc#...
src/natives-external.cc:97: };
nit. DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc
File src/snapshot-external.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:17: class SnapshotImpl {
should be a struct (and use e.g. data_ instead of data)

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:39: static SnapshotImpl* impl_;
and this should be moved out of SnapshotImpl

[Benedikt Meurer, 2014-05-27 04:09:59]

LGTM with nits.

https://codereview.chromium.org/293993021/diff/40001/src/natives.h
File src/natives.h (right):

https://codereview.chromium.org/293993021/diff/40001/src/natives.h#newcode10
src/natives.h:10: namespace v8 { class StartupData; }
Nit: Add comment // Forward declarations.

https://codereview.chromium.org/293993021/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/serialize.cc#newcode19
src/serialize.cc:19: #include "snapshot-source-sink.h"
Nit: serialize.h already includes snapshout-source-sink.h.

https://codereview.chromium.org/293993021/diff/40001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/293993021/diff/40001/src/serialize.h#newcode8
src/serialize.h:8: #include "v8.h"
Nit: Please don't introduce new v8.h includes, but instead, try to include what
you actually need.

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-common.cc
File src/snapshot-common.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-common.cc#n...
src/snapshot-common.cc:55: bool Snapshot::IsEnabled() {
Nit: IsEnabled() should be marked as const.
Why do we need that function at all? Looks like it does the same as
HaveASnapshotToStartFrom()?

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc
File src/snapshot-external.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:7: #include "v8.h"
Nit: No new v8.h includes if possible.

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-source-sink.h
File src/snapshot-source-sink.h (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-source-sink...
src/snapshot-source-sink.h:20: class SnapshotByteSource {
Nit: Mark as V8_FINAL.

[vogelheim, 2014-05-27 15:20:23]

PTAL.

(Had to rebase, so unfortunately the diffs between versions are hard to read. Is
there any way around that?)

https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc
File src/natives-external.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc#...
src/natives-external.cc:1: // Copyright 2011 the V8 project authors. All rights
reserved.
On 2014/05/26 14:34:49, jochen wrote:
> nit 2014

Done.

https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc#...
src/natives-external.cc:97: };
On 2014/05/26 14:34:49, jochen wrote:
> nit. DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/293993021/diff/40001/src/natives.h
File src/natives.h (right):

https://codereview.chromium.org/293993021/diff/40001/src/natives.h#newcode10
src/natives.h:10: namespace v8 { class StartupData; }
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
> Nit: Add comment // Forward declarations.

Done.

https://codereview.chromium.org/293993021/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/serialize.cc#newcode19
src/serialize.cc:19: #include "snapshot-source-sink.h"
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
> Nit: serialize.h already includes snapshout-source-sink.h.

Done.

https://codereview.chromium.org/293993021/diff/40001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/293993021/diff/40001/src/serialize.h#newcode8
src/serialize.h:8: #include "v8.h"
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
> Nit: Please don't introduce new v8.h includes, but instead, try to include
what
> you actually need.

Done.

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-common.cc
File src/snapshot-common.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-common.cc#n...
src/snapshot-common.cc:55: bool Snapshot::IsEnabled() {
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
> Nit: IsEnabled() should be marked as const.
> Why do we need that function at all? Looks like it does the same as
> HaveASnapshotToStartFrom()?

Removed IsEnabled; it's the exact same thing as HaveASnapshotToStartFrom.

It couldn't be const, though, since it's static:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/snapshot.h&...

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc
File src/snapshot-external.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:7: #include "v8.h"
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
> Nit: No new v8.h includes if possible.

Well, I tried, but this uses V8::Initialize(Deserializer*) which is declared in
src/v8.h. How could I get around this?

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/v8.h&l=62

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:17: class SnapshotImpl {
On 2014/05/26 14:34:49, jochen wrote:
> should be a struct (and use e.g. data_ instead of data)

Done.

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:39: static SnapshotImpl* impl_;
On 2014/05/26 14:34:49, jochen wrote:
> and this should be moved out of SnapshotImpl

Done.

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-source-sink.h
File src/snapshot-source-sink.h (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-source-sink...
src/snapshot-source-sink.h:20: class SnapshotByteSource {
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
> Nit: Mark as V8_FINAL.

Done.

[Benedikt Meurer, 2014-05-29 03:19:59]

I usually try to upload a separate REBASE patch, but even that doesn't always
yield readable diffs....

Still LGTM.

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc
File src/snapshot-external.cc (right):

https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc...
src/snapshot-external.cc:7: #include "v8.h"
I see, in that case we'll have to keep that v8.h include for now.

[vogelheim, 2014-06-03 14:38:59]

Committed patchset #6 manually as r21646 (presubmit successful).