Attach snapshot data blob to the isolate.

Attach snapshot data blob to the isolate.

R=vogelheim@chromium.org
BUG=chromium:461259
LOG=N

Committed: https://crrev.com/4b5ff0c5e2a05fbdc559e48646e360da43d98f75
Cr-Commit-Position: refs/heads/master@{#26845}

[vogelheim, 2015-02-24 15:16:31]

Mostly lgtm. Minor comments inline, but here's a problem I'm having:

It used to be that the snapshot was effectively a cache for the compiled
natives. The code in Isolate::New tries to initialize from the snapshot, and
tries again without the snapshot if that fails. Which is just fine.

But now, if custom_source was used, initialization with or without the snapshot
no longer produces the same output. So depending on whether the first
Snapshot::Initialize succeeded or not, the state of the resulting Isolate will
be vastly different, without any way for the caller to know. Which strikes me as
quite an API problem.

(In fairness, that problem existed before. This CL merely makes it more
prominent.)

Honestly, I don't know what to recommend. Maybe someone with more API knowledge
has a better idea. IMHO, options would be:
- leave as is. Whoever uses custom_source better know what they're doing.
- promote deserialization fail to failing Isolate::New.
- .. somehow remember/pass along custom_source, so that initialization from
natives will yield identical results.

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

https://codereview.chromium.org/949623006/diff/1/src/api.cc#newcode6529
src/api.cc:6529: if (!params.enable_serializer) {
Why the check for !enable_serializer?

The comment in v8.h says "This flag currently renders the Isolate unusable",
which both explains and not explains the check.

https://codereview.chromium.org/949623006/diff/1/src/snapshot.h
File src/snapshot.h (right):

https://codereview.chromium.org/949623006/diff/1/src/snapshot.h#newcode39
src/snapshot.h:39: static bool HaveASnapshotToStartFrom(Isolate* isolate) {
nitpick: This (and the methods below) would conceptually be a better fit as
methods on Isolate.

Previously, these methods used to answer a question about the static snapshot
data. But now, they answer a question about the state of the given Isolate.

https://codereview.chromium.org/949623006/diff/1/src/snapshot.h#newcode40
src/snapshot.h:40: // Do not use snapshots if the isolate is used to create
snapshots.
I think this comment explains the enable_serializer check earlier. Maybe move it
there?

https://codereview.chromium.org/949623006/diff/1/test/cctest/test-serialize.cc
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/949623006/diff/1/test/cctest/test-serialize.c...
test/cctest/test-serialize.cc:708: // Disable experimental natives that are
loaded after desrialization.
nitpick: desrialization => deserialization

[Yang, 2015-02-25 08:19:21]

I've looked closer at Isolate::Init, and it seems that it can only return false
in combination of an FatalOutOfMemory, which causes a crash before actually
returning. Nevertheless I added code to handle the case we fail to initialize
from the snapshot and have a script embedded. We now simply crash with a
V8_Fatal.

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

https://codereview.chromium.org/949623006/diff/1/src/api.cc#newcode6529
src/api.cc:6529: if (!params.enable_serializer) {
On 2015/02/24 15:16:31, vogelheim wrote:
> Why the check for !enable_serializer?
> 
> The comment in v8.h says "This flag currently renders the Isolate unusable",
> which both explains and not explains the check.

You are right. In fact, there is no point in having this flag anymore. The only
user of this flag is in V8::CreateSnapshotDataBlob. I'll remove it.

https://codereview.chromium.org/949623006/diff/1/src/snapshot.h
File src/snapshot.h (right):

https://codereview.chromium.org/949623006/diff/1/src/snapshot.h#newcode39
src/snapshot.h:39: static bool HaveASnapshotToStartFrom(Isolate* isolate) {
On 2015/02/24 15:16:31, vogelheim wrote:
> nitpick: This (and the methods below) would conceptually be a better fit as
> methods on Isolate.
> 
> Previously, these methods used to answer a question about the static snapshot
> data. But now, they answer a question about the state of the given Isolate.

Done.

https://codereview.chromium.org/949623006/diff/1/src/snapshot.h#newcode40
src/snapshot.h:40: // Do not use snapshots if the isolate is used to create
snapshots.
On 2015/02/24 15:16:31, vogelheim wrote:
> I think this comment explains the enable_serializer check earlier. Maybe move
it
> there?

Acknowledged.

https://codereview.chromium.org/949623006/diff/1/test/cctest/test-serialize.cc
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/949623006/diff/1/test/cctest/test-serialize.c...
test/cctest/test-serialize.cc:708: // Disable experimental natives that are
loaded after desrialization.
On 2015/02/24 15:16:31, vogelheim wrote:
> nitpick: desrialization => deserialization

Done.

[Yang, 2015-02-25 10:50:17]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-02-25 10:50:18]

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

[commit-bot: I haz the power, 2015-02-25 10:51:33]

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

[commit-bot: I haz the power, 2015-02-25 11:14:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-25 11:15:05]

Patchset 6 (id:??) landed as
https://crrev.com/4b5ff0c5e2a05fbdc559e48646e360da43d98f75
Cr-Commit-Position: refs/heads/master@{#26845}