[snapshot] introduce SnapshotCreator API.

[snapshot] introduce SnapshotCreator API.

And use it to implement existing the existing snapshot API.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
Committed: https://crrev.com/e5760c08086d7d0de9144ca549b824846d718da9
Cr-Commit-Position: refs/heads/master@{#36879}

[Yang, 2016-06-08 12:23:32]

On 2016/06/08 12:22:42, Yang wrote:

Or should we call it SnapshotBuilder?

[jochen (gone - plz use gerrit), 2016-06-08 13:06:48]

both names are fine :)

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

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6497
include/v8.h:6497: class SnapshotCreator {
I'd not make this a subclass of V8 but a top-level class

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6499
include/v8.h:6499: enum FunctionCodeHandling { CLEAR_FUNCTION_CODE,
KEEP_FUNCTION_CODE };
what about using an enum class { kClear, kKeep}

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6506
include/v8.h:6506: explicit SnapshotCreator(StartupData* existing_blob =
nullptr);
can you explain why the existing snapshot is needed?

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6524
include/v8.h:6524: * \returns { NULL, 0 } on failure, and a startup snapshot on
success. The
nullptr?

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6528
include/v8.h:6528: FunctionCodeHandling function_code_handling =
CLEAR_FUNCTION_CODE);
I don't think the default arg is needed

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6532
include/v8.h:6532: };
do we have some DISALLOW_COPY_AND_ASSING in the API?

[Yang, 2016-06-08 13:30:29]

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

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6497
include/v8.h:6497: class SnapshotCreator {
On 2016/06/08 13:06:48, jochen wrote:
> I'd not make this a subclass of V8 but a top-level class

Done.

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6499
include/v8.h:6499: enum FunctionCodeHandling { CLEAR_FUNCTION_CODE,
KEEP_FUNCTION_CODE };
On 2016/06/08 13:06:48, jochen wrote:
> what about using an enum class { kClear, kKeep}

Done.

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6506
include/v8.h:6506: explicit SnapshotCreator(StartupData* existing_blob =
nullptr);
On 2016/06/08 13:06:48, jochen wrote:
> can you explain why the existing snapshot is needed?

As discussed, this is for creating warmed up snapshots.

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6524
include/v8.h:6524: * \returns { NULL, 0 } on failure, and a startup snapshot on
success. The
On 2016/06/08 13:06:48, jochen wrote:
> nullptr? 

Done.

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6528
include/v8.h:6528: FunctionCodeHandling function_code_handling =
CLEAR_FUNCTION_CODE);
On 2016/06/08 13:06:48, jochen wrote:
> I don't think the default arg is needed

Done.

https://codereview.chromium.org/2046423002/diff/1/include/v8.h#newcode6532
include/v8.h:6532: };
On 2016/06/08 13:06:48, jochen wrote:
> do we have some DISALLOW_COPY_AND_ASSING in the API?

We don't have it as a macro, but override the copy constructor and = operator in
some places.

I could also use DISALLOW_COPY_AND_ASSIGN on the ArrayBufferAllocator member to
achieve the same effect...

[Yang, 2016-06-08 13:30:52]

> I could also use DISALLOW_COPY_AND_ASSIGN on the ArrayBufferAllocator member
to
> achieve the same effect...

Never mind this. This is wrong.

[jochen (gone - plz use gerrit), 2016-06-08 15:04:55]

lgtm

[Yang, 2016-06-09 10:32:11]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 10:32:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046423002/20001

[commit-bot: I haz the power, 2016-06-09 10:34:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 10:34:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/7005)
  v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[Yang, 2016-06-09 12:54:47]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 12:54:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046423002/40001

[Yang, 2016-06-09 12:56:37]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 12:56:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046423002/60001

[commit-bot: I haz the power, 2016-06-09 13:01:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 13:01:20]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/2923)

[Yang, 2016-06-09 13:27:10]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 13:27:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046423002/80001

[vogelheim, 2016-06-09 13:42:07]

lgtm

(Sorry for being late. Also, only nitpicks, so ignore if this is already
submitted... :) )

https://codereview.chromium.org/2046423002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2046423002/diff/80001/include/v8.h#newcode6780
include/v8.h:6780: * Create an enter an isolate, which is correctly set up for
serialization.
language nitpicks:

"Create an enter" => "Create and enter" ?

"isolate, which is correctly ..." => "and set it up for serialization."

https://codereview.chromium.org/2046423002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2046423002/diff/80001/src/api.cc#newcode434
src/api.cc:434: Isolate* isolate = data->isolate_;
maybe: DCHECK(!data->created_);

(I take it one could, in theory, add contexts after calling CreateBlob, but one
can't do anything useful with them?)

[commit-bot: I haz the power, 2016-06-09 13:56:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 13:56:07]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-06-10 05:03:47]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-06-10 05:03:47]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2046423002/#ps100001
(title: "address comments")

[commit-bot: I haz the power, 2016-06-10 05:03:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046423002/100001

[commit-bot: I haz the power, 2016-06-10 05:06:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-10 05:06:28]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[Yang, 2016-06-10 05:14:25]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-06-10 05:14:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2046423002/#ps120001
(title: "fix")

[commit-bot: I haz the power, 2016-06-10 05:14:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046423002/120001

[commit-bot: I haz the power, 2016-06-10 05:44:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-10 05:47:58]

Description was changed from

==========
[snapshot] introduce SnapshotCreator API.

And use it to implement existing the existing snapshot API.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892
==========

to

==========
[snapshot] introduce SnapshotCreator API.

And use it to implement existing the existing snapshot API.

R=jochen@chromium.org, vogelheim@chromium.org
BUG=chromium:617892

Committed: https://crrev.com/e5760c08086d7d0de9144ca549b824846d718da9
Cr-Commit-Position: refs/heads/master@{#36879}
==========

[commit-bot: I haz the power, 2016-06-10 05:47:59]

Patchset 7 (id:??) landed as
https://crrev.com/e5760c08086d7d0de9144ca549b824846d718da9
Cr-Commit-Position: refs/heads/master@{#36879}