[heap] Record references in the new code objects in heap::CopyCode.

[heap] Record references in the new code objects in heap::CopyCode.

R=mlippautz@chromium.org
BUG=chromium:633539
TEST=cctest/test-heap/TestNewSpaceRefsInCopiedCode

Committed: https://crrev.com/c088aea9222c312a78e9eae26fab7ffd57704c2e
Cr-Commit-Position: refs/heads/master@{#38326}

[ahaas, 2016-08-02 14:36:46]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 14:36:49]

Dry run: 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, 2016-08-02 14:55:20]

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

[commit-bot: I haz the power, 2016-08-02 14:55:22]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/11673)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Michael Lippautz, 2016-08-02 19:07:57]

mlippautz@chromium.org changed reviewers:
+ ulan@chromium.org

[Michael Lippautz, 2016-08-02 19:07:58]

+ulan

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc#newcode3377
src/heap/heap.cc:3377: for (RelocIterator it(new_code,
That works, but ideally we would have fused visitor (together with the code in
IterateBlackObject). In the case that we copy a black object you need to visit
the reloc info twice, which is not ideal. Then again CopyCode might be rare
enough so that it does not matter.

Ulan, wdyt?

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h#newcode1870
src/heap/heap.h:1870: friend AllocationResult TestCopyCode(Heap* heap, Code*
code);
No need to do that. Have a look at
  test/cctest/heap/heap-tester.h
. Then just declare the test as HEAP_TEST(YourTest) {} and you have access to
internals.

https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.cc
File test/cctest/heap/test-heap.cc (right):

https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap....
test/cctest/heap/test-heap.cc:126: Handle<Object> value =
factory->NewNumber(1.000123);
CHECK for object being in new space

https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap....
test/cctest/heap/test-heap.cc:136: Handle<Code> code =
isolate->factory()->NewCode(
Cool stuff!

[ahaas, 2016-08-03 07:57:53]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 07:57:59]

Dry run: 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, 2016-08-03 08:18:27]

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

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

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-03 08:43:47]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 08:43:49]

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

[ahaas, 2016-08-03 08:47:42]

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc#newcode3377
src/heap/heap.cc:3377: for (RelocIterator it(new_code,
On 2016/08/02 at 19:07:57, Michael Lippautz wrote:
> That works, but ideally we would have fused visitor (together with the code in
IterateBlackObject). In the case that we copy a black object you need to visit
the reloc info twice, which is not ideal. Then again CopyCode might be rare
enough so that it does not matter.
> 
> Ulan, wdyt?

I think that this would be an optimization that we should not do unless there
are actual performance reasons for it.

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.h#newcode1870
src/heap/heap.h:1870: friend AllocationResult TestCopyCode(Heap* heap, Code*
code);
On 2016/08/02 at 19:07:57, Michael Lippautz wrote:
> No need to do that. Have a look at
>   test/cctest/heap/heap-tester.h
> . Then just declare the test as HEAP_TEST(YourTest) {} and you have access to
internals.

Done.

https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap.cc
File test/cctest/heap/test-heap.cc (right):

https://codereview.chromium.org/2203783002/diff/1/test/cctest/heap/test-heap....
test/cctest/heap/test-heap.cc:126: Handle<Object> value =
factory->NewNumber(1.000123);
On 2016/08/02 at 19:07:58, Michael Lippautz wrote:
> CHECK for object being in new space

Done.

[commit-bot: I haz the power, 2016-08-03 09:02:35]

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

[commit-bot: I haz the power, 2016-08-03 09:02:36]

Dry run: This issue passed the CQ dry run.

[ulan, 2016-08-03 10:59:32]

lgtm

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2203783002/diff/1/src/heap/heap.cc#newcode3377
src/heap/heap.cc:3377: for (RelocIterator it(new_code,
On 2016/08/03 08:47:42, ahaas wrote:
> On 2016/08/02 at 19:07:57, Michael Lippautz wrote:
> > That works, but ideally we would have fused visitor (together with the code
in
> IterateBlackObject). In the case that we copy a black object you need to visit
> the reloc info twice, which is not ideal. Then again CopyCode might be rare
> enough so that it does not matter.
> > 
> > Ulan, wdyt?
> 
> I think that this would be an optimization that we should not do unless there
> are actual performance reasons for it.

Yep, I would prefer decoding reloc info once. But for the sake of simplicity
let's keep it as it is unless benchmarks regress.

In any case, could you please extract new code into RecordWritesIntoCode
function?

[Michael Lippautz, 2016-08-03 11:25:57]

lgtm once Ulan's comment is addressed.

[ahaas, 2016-08-03 16:08:22]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 16:08:30]

Dry run: 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, 2016-08-03 16:30:00]

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

[commit-bot: I haz the power, 2016-08-03 16:30:01]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-04 07:22:02]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 07:22:16]

Dry run: 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, 2016-08-04 07:43:23]

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

[commit-bot: I haz the power, 2016-08-04 07:43:24]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-04 08:10:39]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-08-04 08:10:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org, ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/2203783002/#ps80001
(title: "Init tmp pointer to avoid compiler warning")

[commit-bot: I haz the power, 2016-08-04 08:10:45]

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, 2016-08-04 08:12:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-04 08:14:42]

Description was changed from

==========
[heap] Record references in the new code objects in heap::CopyCode.

R=mlippautz@chromium.org
BUG=chromium:633539
TEST=cctest/test-heap/TestNewSpaceRefsInCopiedCode
==========

to

==========
[heap] Record references in the new code objects in heap::CopyCode.

R=mlippautz@chromium.org
BUG=chromium:633539
TEST=cctest/test-heap/TestNewSpaceRefsInCopiedCode

Committed: https://crrev.com/c088aea9222c312a78e9eae26fab7ffd57704c2e
Cr-Commit-Position: refs/heads/master@{#38326}
==========

[commit-bot: I haz the power, 2016-08-04 08:14:43]

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