Special case ConstantPoolArray in MarkCompactCollector::MigrateObject.

Special case ConstantPoolArray in MarkCompactCollector::MigrateObject.

Special case the ConstantPoolArray in MarkCompactCollector::MigrateObject since it could contain
integer value entires which look like tagged pointers.

R=hpayer@chromium.org, mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21934

[rmcilroy, 2014-05-29 15:54:12]

Given that ConstantPoolArrays can contain raw int32 or int64 values, I think
this change is necessary to ensure we don't confuse a raw int value as a tagged
pointer when migrating a ConstantPoolArray.  PTAL.

[rmcilroy, 2014-06-03 12:36:10]

On 2014/05/29 15:54:12, rmcilroy wrote:
> Given that ConstantPoolArrays can contain raw int32 or int64 values, I think
> this change is necessary to ensure we don't confuse a raw int value as a
tagged
> pointer when migrating a ConstantPoolArray.  PTAL.

Ping? Could you take a look please Michael?

[Michael Starzinger, 2014-06-03 22:57:12]

LGTM (clenching teeth) to fix the bug, but this special casing is getting out of
hand and the code needs to be restructured soon.

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2876
src/mark-compact.cc:2876: if (!(compacting_ && dst->IsConstantPoolArray())) {
OMG, this is a giant hack. Please leave a TODO(mstarzinger) here that this code
needs to be re-structured somehow. I haven't thought about it long enough to
give constructive feedback yet.

[Michael Starzinger, 2014-06-03 23:03:50]

+Hannes, I would like him to also sign off on this.

[Hannes Payer (out of office), 2014-06-04 07:25:04]

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2874
src/mark-compact.cc:2874: // We special case ConstantPoolArrays below since they
could contain
contain contain => contain

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2876
src/mark-compact.cc:2876: if (!(compacting_ && dst->IsConstantPoolArray())) {
On 2014/06/03 22:57:11, Michael Starzinger wrote:
> OMG, this is a giant hack. Please leave a TODO(mstarzinger) here that this
code
> needs to be re-structured somehow. I haven't thought about it long enough to
> give constructive feedback yet.

The problem is that constant pools do not belong to old pointer space or old
data space, they can contain both data and pointers. This is reason why we have
to do the special handling left and right. Merging both old spaces may help
cleaning this up.

I am not happy with this hack, but there is not much we can do right now.

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2876
src/mark-compact.cc:2876: if (!(compacting_ && dst->IsConstantPoolArray())) {
if (!(compacting_ && dst->IsConstantPoolArray()))
=>
if (!compacting_ || !dst->IsConstantPoolArray())

Why do you need !compacting_? With that you would still create store buffer
entries for constant pool entries that look like tagged pointers.

[rmcilroy, 2014-06-04 10:15:57]

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2876
src/mark-compact.cc:2876: if (!(compacting_ && dst->IsConstantPoolArray())) {
On 2014/06/03 22:57:11, Michael Starzinger wrote:
> OMG, this is a giant hack. Please leave a TODO(mstarzinger) here that this
code
> needs to be re-structured somehow. I haven't thought about it long enough to
> give constructive feedback yet.

Done.

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2876
src/mark-compact.cc:2876: if (!(compacting_ && dst->IsConstantPoolArray())) {
On 2014/06/04 07:25:04, Hannes Payer wrote:
> if (!(compacting_ && dst->IsConstantPoolArray()))
> =>
> if (!compacting_ || !dst->IsConstantPoolArray())
> 
> Why do you need !compacting_? With that you would still create store buffer
> entries for constant pool entries that look like tagged pointers.

ConstantPoolArrays are pre-tenured, so they should only ever be being migrated
when "compacting_".  I removed the check in anycase since it shouldn't make a
difference either way.  Also changed the check to src->IsConstantPoolArray() to
avoid issues if the compiler aggressively moves this statement before the
"Memory::Object_at(dst_slot) = value" above.

[rmcilroy, 2014-06-04 10:19:26]

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2874
src/mark-compact.cc:2874: // We special case ConstantPoolArrays below since they
could contain
On 2014/06/04 07:25:04, Hannes Payer wrote:
> contain contain => contain

Done.

[Hannes Payer (out of office), 2014-06-04 10:54:16]

lgtm

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/1/src/mark-compact.cc#newcode2876
src/mark-compact.cc:2876: if (!(compacting_ && dst->IsConstantPoolArray())) {
On 2014/06/04 10:15:56, rmcilroy wrote:
> On 2014/06/04 07:25:04, Hannes Payer wrote:
> > if (!(compacting_ && dst->IsConstantPoolArray()))
> > =>
> > if (!compacting_ || !dst->IsConstantPoolArray())
> > 
> > Why do you need !compacting_? With that you would still create store buffer
> > entries for constant pool entries that look like tagged pointers.
> 
> ConstantPoolArrays are pre-tenured, so they should only ever be being migrated
> when "compacting_".  I removed the check in anycase since it shouldn't make a
> difference either way.  Also changed the check to src->IsConstantPoolArray()
to
> avoid issues if the compiler aggressively moves this statement before the
> "Memory::Object_at(dst_slot) = value" above.

I was worried about creating store buffer entries for int values that look like
new space pointers.

https://codereview.chromium.org/304223002/diff/40001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/40001/src/mark-compact.cc#newc...
src/mark-compact.cc:2859: } else if (dst->IsConstantPoolArray()) {
You could keep compacting_ here, but it doesn't really matter. As you like.

[Hannes Payer (out of office), 2014-06-04 11:01:58]

https://codereview.chromium.org/304223002/diff/40001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/304223002/diff/40001/src/mark-compact.cc#newc...
src/mark-compact.cc:2859: } else if (dst->IsConstantPoolArray()) {
On 2014/06/04 10:54:16, Hannes Payer wrote:
> You could keep compacting_ here, but it doesn't really matter. As you like.

Not true, ignore this comment.

[rmcilroy, 2014-06-23 12:18:20]

Committed patchset #3 manually as r21934 (presubmit successful).