Serializer: serialize internal references via object visitor.

Serializer: serialize internal references via object visitor.

Committed: https://crrev.com/7c149afb6c875b1c53723384459dc14a0e961927
Cr-Commit-Position: refs/heads/master@{#27275}

[Yang, 2015-03-17 15:03:03]

yangguo@chromium.org changed reviewers:
+ erik.corry@gmail.com

[Yang, 2015-03-17 15:03:04]

We used to iterate the reloc info an extra time to turn internal references into
relative addresses when serializing, and again when deserializing to turn
relative addresses into absolute addresses.

Since we iterate the reloc info anyways, we now emit opcode for internal
references. This uses a bit more space, but is faster and internal references
are rather rare anyways.

Platform ports to ARM are coming up. This is for the first round of comments.

[Erik Corry, 2015-03-17 15:31:16]

Looks basically sound

https://codereview.chromium.org/1005183006/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/1005183006/diff/1/src/serialize.cc#newcode1875
src/serialize.cc:1875: DCHECK(code_object_);
I don't think you are allowed to use implicit bool conversion.

https://codereview.chromium.org/1005183006/diff/1/src/serialize.cc#newcode1970
src/serialize.cc:1970: if (!(FLAG_enable_ool_constant_pool &&
rinfo->IsInConstantPool())) {
If you don't wipe internal offsets, will snapshots still be reproducible?  This
would only be the case if they were always coded in a relative way, but in that
case you didn't need special handling in the snapshot at all.

[Yang, 2015-03-18 09:25:49]

I added platform ports and changed the encoding scheme. ARM64 is incomplete
because we don't use internal references there to begin with.

In the first patch set, I used the skip encoding to find the code position to
patch, just like other embedded references. This assumes (and we assert) that
skip is always positive, i.e. the code positions to patch is in ascending order.
This means that code positions holding embedded references in the reloc info are
in ascending order. This is not true if we toss in internal references, which
are inline in the code, and have e.g. external references stored in a constant
pool at the end of the code object.

So instead of using this skip encoding, I always encode internal references (pc
mapping to target pc) as two offsets from code entry. This way it does not mess
with the skip encoding of other embedded references.

https://codereview.chromium.org/1005183006/diff/20001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/1005183006/diff/20001/src/serialize.cc#newcod...
src/serialize.cc:2003: DCHECK(to_skip >= 0);
This is the assertion I was referring to.

[Yang, 2015-03-18 09:31:55]

For MIPS and PPC ports of this patch: you will have to distinguish between
INTERNAL_REFERENCE and INTERNAL_REFERENCE_ENCODED, but you should be able to
implement that on platform-dependent files.

[Yang, 2015-03-18 09:37:20]

https://codereview.chromium.org/1005183006/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/1005183006/diff/1/src/serialize.cc#newcode1875
src/serialize.cc:1875: DCHECK(code_object_);
On 2015/03/17 15:31:16, Erik Corry wrote:
> I don't think you are allowed to use implicit bool conversion.

code_object_ is a bool. I renamed it to is_code_object_

https://codereview.chromium.org/1005183006/diff/1/src/serialize.cc#newcode1970
src/serialize.cc:1970: if (!(FLAG_enable_ool_constant_pool &&
rinfo->IsInConstantPool())) {
On 2015/03/17 15:31:16, Erik Corry wrote:
> If you don't wipe internal offsets, will snapshots still be reproducible? 
This
> would only be the case if they were always coded in a relative way, but in
that
> case you didn't need special handling in the snapshot at all.

This is necessary to make the snapshot reproducible. Internal references are
absolute addresses into the code object (currently only used for jump tables),
and therefore need to change on relocation.

[Erik Corry, 2015-03-18 10:59:18]

lgtm

[Yang, 2015-03-18 12:24:16]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-03-18 12:24:24]

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

[commit-bot: I haz the power, 2015-03-18 12:24:25]

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

[commit-bot: I haz the power, 2015-03-18 12:24:26]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mvstanton, 2015-03-18 12:26:55]

mvstanton@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2015-03-18 12:26:56]

rubber stamp, reviewed by Erik.
LGTM.

[Yang, 2015-03-18 12:27:52]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-03-18 12:28:08]

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

[commit-bot: I haz the power, 2015-03-18 12:31:03]

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

[commit-bot: I haz the power, 2015-03-18 12:31:04]

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

[Yang, 2015-03-18 12:35:50]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-03-18 12:35:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, erik.corry@gmail.com
Link to the patchset: https://codereview.chromium.org/1005183006/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-18 12:36:02]

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

[commit-bot: I haz the power, 2015-03-18 12:39:30]

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

[commit-bot: I haz the power, 2015-03-18 12:39:31]

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

[Yang, 2015-03-18 12:52:24]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-03-18 12:52:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, erik.corry@gmail.com
Link to the patchset: https://codereview.chromium.org/1005183006/#ps80001
(title: "fix compilation")

[commit-bot: I haz the power, 2015-03-18 12:52:34]

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

[commit-bot: I haz the power, 2015-03-18 13:38:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-18 13:39:00]

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