[serializer] Ensure immortal immovable roots are deserialized correctly.

[serializer] Ensure immortal immovable roots are deserialized correctly.

The serializer collects objects in iteration order, not in allocation
order. This means that the deserializer will put these objects in
iteration order onto the reserved pages as well. There is no guarantee
that objects that were on the first page will end up on the first page
after deserialization.

Until now we got lucky, since we only ever need one space per page for
the default snapshot. For roots, the iteration order and allocation
order also do not differ enough to cause any issue for immortal
immovable root objects. These objects need to stay on the first page of
its allocated space to not move.

However, let's make sure it stays this way, and we realize soon enough
if this assumption does not hold.

R=mstarzinger@chromium.org

Committed: https://crrev.com/07d40b74b6e33df99e5d52de4fe56893568fa16d
Cr-Commit-Position: refs/heads/master@{#33810}

[Michael Starzinger, 2016-02-05 14:47:59]

https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc
File src/address-map.cc (right):

https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc#newcode36
src/address-map.cc:36: CHECK(!Heap::RootIsImmortalImmovable(root_index));
I am not sure about this check. It only holds because we don't have e.g.
constant Smi entries in the root-set. Is this check really necessary to ensure
the invariant in question in the deserializer? Otherwise I would vote for
dropping this check.

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

https://codereview.chromium.org/1675553002/diff/1/src/snapshot/serialize.cc#n...
src/snapshot/serialize.cc:1741: // chunk of its reserved space , so that it is
deserialized onto the first
nit: Spurious white-space before comma.

[Yang, 2016-02-05 14:55:23]

On 2016/02/05 14:47:59, Michael Starzinger wrote:
> https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc
> File src/address-map.cc (right):
> 
> https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc#newcode36
> src/address-map.cc:36: CHECK(!Heap::RootIsImmortalImmovable(root_index));
> I am not sure about this check. It only holds because we don't have e.g.
> constant Smi entries in the root-set. Is this check really necessary to ensure
> the invariant in question in the deserializer? Otherwise I would vote for
> dropping this check.
> 
> https://codereview.chromium.org/1675553002/diff/1/src/snapshot/serialize.cc
> File src/snapshot/serialize.cc (right):
> 
>
https://codereview.chromium.org/1675553002/diff/1/src/snapshot/serialize.cc#n...
> src/snapshot/serialize.cc:1741: // chunk of its reserved space , so that it is
> deserialized onto the first
> nit: Spurious white-space before comma.

We use the root index map to find immortal immovable roots when we serialize, in
order to run the check. This check here makes sure we dont miss one.

[Michael Starzinger, 2016-02-05 15:03:21]

Fair enough, fine with me, just one last suggestion to maybe avoid this
confusion. Otherwise LGTM.

https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc
File src/address-map.cc (right):

https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc#newcode23
src/address-map.cc:23: isolate->heap()->RootCanBeTreatedAsConstant(root_index))
{
Would it be possible to rephrase this as follows then? ...

if (isolate->heap()->RootCanBeTreatedAsConstant(root_index)) {
  DCHECK(root->IsHeapObject);  // No constant Smi's in root set.
  HeapObject* heap_object = HeapObject::cast(root);
  [...]
}

[Yang, 2016-02-08 07:53:47]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-08 07:53:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1675553002/#ps20001
(title: "addressed comment")

[commit-bot: I haz the power, 2016-02-08 07:53:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675553002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675553002/20001

[Yang, 2016-02-08 07:53:59]

https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc
File src/address-map.cc (right):

https://codereview.chromium.org/1675553002/diff/1/src/address-map.cc#newcode23
src/address-map.cc:23: isolate->heap()->RootCanBeTreatedAsConstant(root_index))
{
On 2016/02/05 15:03:21, Michael Starzinger wrote:
> Would it be possible to rephrase this as follows then? ...
> 
> if (isolate->heap()->RootCanBeTreatedAsConstant(root_index)) {
>   DCHECK(root->IsHeapObject);  // No constant Smi's in root set.
>   HeapObject* heap_object = HeapObject::cast(root);
>   [...]
> }

Done.

[commit-bot: I haz the power, 2016-02-08 07:57:38]

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

[commit-bot: I haz the power, 2016-02-08 07:57:39]

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/15111)

[Yang, 2016-02-08 10:26:50]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-08 10:26:50]

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

[commit-bot: I haz the power, 2016-02-08 10:26:58]

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

[commit-bot: I haz the power, 2016-02-08 10:48:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-08 10:49:05]

Description was changed from

==========
[serializer] Ensure immortal immovable roots are deserialized correctly.

The serializer collects objects in iteration order, not in allocation
order. This means that the deserializer will put these objects in
iteration order onto the reserved pages as well. There is no guarantee
that objects that were on the first page will end up on the first page
after deserialization.

Until now we got lucky, since we only ever need one space per page for
the default snapshot. For roots, the iteration order and allocation
order also do not differ enough to cause any issue for immortal
immovable root objects. These objects need to stay on the first page of
its allocated space to not move.

However, let's make sure it stays this way, and we realize soon enough
if this assumption does not hold.

R=mstarzinger@chromium.org
==========

to

==========
[serializer] Ensure immortal immovable roots are deserialized correctly.

The serializer collects objects in iteration order, not in allocation
order. This means that the deserializer will put these objects in
iteration order onto the reserved pages as well. There is no guarantee
that objects that were on the first page will end up on the first page
after deserialization.

Until now we got lucky, since we only ever need one space per page for
the default snapshot. For roots, the iteration order and allocation
order also do not differ enough to cause any issue for immortal
immovable root objects. These objects need to stay on the first page of
its allocated space to not move.

However, let's make sure it stays this way, and we realize soon enough
if this assumption does not hold.

R=mstarzinger@chromium.org

Committed: https://crrev.com/07d40b74b6e33df99e5d52de4fe56893568fa16d
Cr-Commit-Position: refs/heads/master@{#33810}
==========

[commit-bot: I haz the power, 2016-02-08 10:49:06]

Patchset 3 (id:??) landed as
https://crrev.com/07d40b74b6e33df99e5d52de4fe56893568fa16d
Cr-Commit-Position: refs/heads/master@{#33810}