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.cc#newcode708
test/cctest/test-serialize.cc:708: // Disable experimental natives that
are loaded after desrialization.
nitpick: desrialization => deserialization
https://codereview.chromium.org/949623006/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
