SnapshotCreator: start from existing snapshot if we have one

SnapshotCreator: start from existing snapshot if we have one

This requires serialized data to track the number of API-provided
external references separately.
And it flushes out a case of serialized data corruption (stored "length"
field too large) that we didn't handle without crashing.

BUG=v8:6055
Review-Url: https://codereview.chromium.org/2736923002
Cr-Commit-Position: refs/heads/master@{#43649}
Committed: https://chromium.googlesource.com/v8/v8/+/c478a2298d145772fbb848e39627e304f0a5c60a

[Jakob Kummerow, 2017-03-07 10:30:57]

jkummerow@chromium.org changed reviewers:
+ yangguo@chromium.org

[Jakob Kummerow, 2017-03-07 10:30:58]

PTAL. If you would prefer a different solution, I'm open to discussion :-)

[Yang, 2017-03-07 11:28:12]

LGTM with comments.

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

https://codereview.chromium.org/2736923002/diff/1/src/api.cc#newcode520
src/api.cc:520: const StartupData* snapshot_blob =
i::Snapshot::DefaultSnapshotBlob();
We could also make this the default parameter for existing_snapshot?

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
src/snapshot/code-serializer.cc:373: int padded_payload_offset =
POINTER_SIZE_ALIGN(payload_offset);
The reason we aligned is for checksum computation. It used to be that the
reservations and stub keys were not checksummed. They are now, because we
checksum everything from DataWithoutHeader. If we make sure that is aligned, and
you are, we can remove the alignment here.

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
src/snapshot/code-serializer.cc:432: POINTER_SIZE_ALIGN(kHeaderSize +
See above. Let's not align.

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serializer.h
File src/snapshot/code-serializer.h (right):

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
src/snapshot/code-serializer.h:143: const int kPaddedStart =
POINTER_SIZE_ALIGN(kHeaderSize);
Let's define kHeaderSize to be padded. Also see reason below.

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

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/snapshot.h#new...
src/snapshot/snapshot.h:49: static const int kCheckSumOffset =
Now that I see it, this name is a bit misleading. Can we either call this
kVersionHashOffset, or move this and the one in SerializedCodeData into
SerializedData?

https://codereview.chromium.org/2736923002/diff/1/tools/dev/gm.py
File tools/dev/gm.py (right):

https://codereview.chromium.org/2736923002/diff/1/tools/dev/gm.py#newcode291
tools/dev/gm.py:291: if return_code == 0:
Is this a stray edit or intentional drive-by fix?

[Jakob Kummerow, 2017-03-07 12:39:15]

Thanks, comments addressed.

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

https://codereview.chromium.org/2736923002/diff/1/src/api.cc#newcode520
src/api.cc:520: const StartupData* snapshot_blob =
i::Snapshot::DefaultSnapshotBlob();
On 2017/03/07 11:28:11, Yang wrote:
> We could also make this the default parameter for existing_snapshot?

Done.

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
src/snapshot/code-serializer.cc:373: int padded_payload_offset =
POINTER_SIZE_ALIGN(payload_offset);
On 2017/03/07 11:28:11, Yang wrote:
> The reason we aligned is for checksum computation. It used to be that the
> reservations and stub keys were not checksummed. They are now, because we
> checksum everything from DataWithoutHeader. If we make sure that is aligned,
and
> you are, we can remove the alignment here.

As discussed offline, checksumming still relies on alignment here.

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
src/snapshot/code-serializer.cc:432: POINTER_SIZE_ALIGN(kHeaderSize +
On 2017/03/07 11:28:11, Yang wrote:
> See above. Let's not align.

See above -- if we align one, we need to align the other too.

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serializer.h
File src/snapshot/code-serializer.h (right):

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/code-serialize...
src/snapshot/code-serializer.h:143: const int kPaddedStart =
POINTER_SIZE_ALIGN(kHeaderSize);
On 2017/03/07 11:28:11, Yang wrote:
> Let's define kHeaderSize to be padded. Also see reason below.

Done.

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

https://codereview.chromium.org/2736923002/diff/1/src/snapshot/snapshot.h#new...
src/snapshot/snapshot.h:49: static const int kCheckSumOffset =
On 2017/03/07 11:28:11, Yang wrote:
> Now that I see it, this name is a bit misleading. Can we either call this
> kVersionHashOffset, or move this and the one in SerializedCodeData into
> SerializedData?

Done (moved).

https://codereview.chromium.org/2736923002/diff/1/tools/dev/gm.py
File tools/dev/gm.py (right):

https://codereview.chromium.org/2736923002/diff/1/tools/dev/gm.py#newcode291
tools/dev/gm.py:291: if return_code == 0:
On 2017/03/07 11:28:11, Yang wrote:
> Is this a stray edit or intentional drive-by fix?

Intentional fix. Running tests doesn't make sense if the build failed.

[Yang, 2017-03-07 12:43:08]

LGTM. thanks for the additional clean up.

[Jakob Kummerow, 2017-03-07 16:12:21]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2017-03-07 16:12:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 16:36:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488903141765800,
"parent_rev": "5a36af3ceb57231a6f2eb3bc385686f0facd6762", "commit_rev":
"c478a2298d145772fbb848e39627e304f0a5c60a"}

[commit-bot: I haz the power, 2017-03-07 16:36:55]

Description was changed from

==========
SnapshotCreator: start from existing snapshot if we have one

This requires serialized data to track the number of API-provided
external references separately.
And it flushes out a case of serialized data corruption (stored "length"
field too large) that we didn't handle without crashing.

BUG=v8:6055
==========

to

==========
SnapshotCreator: start from existing snapshot if we have one

This requires serialized data to track the number of API-provided
external references separately.
And it flushes out a case of serialized data corruption (stored "length"
field too large) that we didn't handle without crashing.

BUG=v8:6055

Review-Url: https://codereview.chromium.org/2736923002
Cr-Commit-Position: refs/heads/master@{#43649}
Committed:
https://chromium.googlesource.com/v8/v8/+/c478a2298d145772fbb848e39627e304f0a...
==========

[commit-bot: I haz the power, 2017-03-07 16:36:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/c478a2298d145772fbb848e39627e304f0a...