[heap] Avoid the use of cells to point from code to new-space objects.

[heap] Avoid the use of cells to point from code to new-space objects.

Cells were needed originally because there was no typed remembered set to
record direct pointers from code space to new space. A previous
CL (https://codereview.chromium.org/2003553002/) already introduced
the remembered set, this CL uses it.

This CL
* stores direct pointers in code objects, even if the target is in new space,
* records the slot of the pointer in typed-old-to-new remembered set,
* adds a list which stores weak code-to-new-space references,
* adds a test to test-heap.cc for weak code-to-new-space references,
* removes prints in tail-call-megatest.js

R=ulan@chromium.org

Committed: https://crrev.com/2d2087b79a293a92a6ed34a2775e481ff2173b3c
Cr-Commit-Position: refs/heads/master@{#37134}

[ulan, 2016-06-08 14:37:00]

Getting there :) Few comments.

https://codereview.chromium.org/2045263002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2045263002/diff/1/src/compiler.cc#newcode290
src/compiler.cc:290: heap->RecordWeakCodeToNewSpaceReference(object, cell);
Could you rename it to AddWeakNewSpaceObjectToCodeDependency(object, cell) ?

To be consistent with the other function.

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

https://codereview.chromium.org/2045263002/diff/1/src/heap/heap.h#newcode195
src/heap/heap.h:195: V(ArrayList, weak_code_to_new_space_ref_list,
WeakCodeToNewSpaceRefList)     \
Lets name it weak_new_space_object_to_code_list.

https://codereview.chromium.org/2045263002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking-inl.h (right):

https://codereview.chromium.org/2045263002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking-inl.h:33: RecordWriteIntoCodeToNewSpaceSlow(host,
rinfo, value);
This won't work because RecordWriteIntoCode can be guarded with IsMarking by
callers.

Please inspect callers of RecordWriteIntoCode that are outside of
IncrementalMarking and add Heap::RecordWriteIntoCode() there.

https://codereview.chromium.org/2045263002/diff/1/src/heap/mark-compact.h
File src/heap/mark-compact.h (right):

https://codereview.chromium.org/2045263002/diff/1/src/heap/mark-compact.h#new...
src/heap/mark-compact.h:544: void RecordRelocSlotToNewSpace(Code* host,
RelocInfo* rinfo, Object* target);
This function doesn't belong to incremental marking and mark compactor because
it is recording slot for scavenger.

Let's move it to Heap new  Heap::RecordWrite and call it 
Heap::RecordWriteToCode.

https://codereview.chromium.org/2045263002/diff/1/src/heap/remembered-set.cc
File src/heap/remembered-set.cc (right):

https://codereview.chromium.org/2045263002/diff/1/src/heap/remembered-set.cc#...
src/heap/remembered-set.cc:45: if (IsValidTypedSlot(heap, chunk, type,
host_addr, addr)) {
Just use Marking::IsBlack(Marking::MarkBitFrom(host_addr) instead of
IsValidTypedSlot.

If the code is alive the slot will be valid.

[ahaas, 2016-06-09 10:34:33]

https://codereview.chromium.org/2045263002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2045263002/diff/1/src/compiler.cc#newcode290
src/compiler.cc:290: heap->RecordWeakCodeToNewSpaceReference(object, cell);
On 2016/06/08 at 14:37:00, ulan wrote:
> Could you rename it to AddWeakNewSpaceObjectToCodeDependency(object, cell) ?
> 
> To be consistent with the other function.

Done. I named it differently because no DependentCode is created in this case.

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

https://codereview.chromium.org/2045263002/diff/1/src/heap/heap.h#newcode195
src/heap/heap.h:195: V(ArrayList, weak_code_to_new_space_ref_list,
WeakCodeToNewSpaceRefList)     \
On 2016/06/08 at 14:37:00, ulan wrote:
> Lets name it weak_new_space_object_to_code_list.

Done.

https://codereview.chromium.org/2045263002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking-inl.h (right):

https://codereview.chromium.org/2045263002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking-inl.h:33: RecordWriteIntoCodeToNewSpaceSlow(host,
rinfo, value);
On 2016/06/08 at 14:37:00, ulan wrote:
> This won't work because RecordWriteIntoCode can be guarded with IsMarking by
callers.
> 
> Please inspect callers of RecordWriteIntoCode that are outside of
IncrementalMarking and add Heap::RecordWriteIntoCode() there.

Done.

https://codereview.chromium.org/2045263002/diff/1/src/heap/mark-compact.h
File src/heap/mark-compact.h (right):

https://codereview.chromium.org/2045263002/diff/1/src/heap/mark-compact.h#new...
src/heap/mark-compact.h:544: void RecordRelocSlotToNewSpace(Code* host,
RelocInfo* rinfo, Object* target);
On 2016/06/08 at 14:37:00, ulan wrote:
> This function doesn't belong to incremental marking and mark compactor because
it is recording slot for scavenger.
> 
> Let's move it to Heap new  Heap::RecordWrite and call it 
Heap::RecordWriteToCode.

Done. I had to put the implementation into heap.cc because of the inline header
nightmare.

https://codereview.chromium.org/2045263002/diff/1/src/heap/remembered-set.cc
File src/heap/remembered-set.cc (right):

https://codereview.chromium.org/2045263002/diff/1/src/heap/remembered-set.cc#...
src/heap/remembered-set.cc:45: if (IsValidTypedSlot(heap, chunk, type,
host_addr, addr)) {
On 2016/06/08 at 14:37:00, ulan wrote:
> Just use Marking::IsBlack(Marking::MarkBitFrom(host_addr) instead of
IsValidTypedSlot.
> 
> If the code is alive the slot will be valid.

Done.

[Michael Achenbach, 2016-06-09 11:08:22]

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

[Michael Achenbach, 2016-06-09 11:08:24]

https://codereview.chromium.org/2045263002/diff/40001/test/mjsunit/es6/tail-c...
File test/mjsunit/es6/tail-call-megatest.js (right):

https://codereview.chromium.org/2045263002/diff/40001/test/mjsunit/es6/tail-c...
test/mjsunit/es6/tail-call-megatest.js:369: //    print("Running shard #" +
shard);
Thanks for the speed-up. Could this not land in a separate CL?

[titzer, 2016-06-09 11:10:21]

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

[titzer, 2016-06-09 11:10:22]

Looks mostly good

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

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2472: list->Set(counter, obj, SKIP_WRITE_BARRIER);
I don't understand why this is a SKIP_WRITE_BARRIER and then a RecordSlot
directly afterwards. Would a full barrier just do the right thing?

https://codereview.chromium.org/2045263002/diff/40001/test/mjsunit/es6/tail-c...
File test/mjsunit/es6/tail-call-megatest.js (right):

https://codereview.chromium.org/2045263002/diff/40001/test/mjsunit/es6/tail-c...
test/mjsunit/es6/tail-call-megatest.js:369: //    print("Running shard #" +
shard);
On 2016/06/09 11:08:23, Michael Achenbach wrote:
> Thanks for the speed-up. Could this not land in a separate CL?

+1

[ahaas, 2016-06-09 11:16:35]

On 2016/06/09 at 11:08:24, machenbach wrote:
>
https://codereview.chromium.org/2045263002/diff/40001/test/mjsunit/es6/tail-c...
> File test/mjsunit/es6/tail-call-megatest.js (right):
> 
>
https://codereview.chromium.org/2045263002/diff/40001/test/mjsunit/es6/tail-c...
> test/mjsunit/es6/tail-call-megatest.js:369: //    print("Running shard #" +
shard);
> Thanks for the speed-up. Could this not land in a separate CL?

I created a separate CL now for the changes in tail-call-megatest.js
https://codereview.chromium.org/2045253006/

[ahaas, 2016-06-09 11:20:40]

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

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2472: list->Set(counter, obj, SKIP_WRITE_BARRIER);
On 2016/06/09 at 11:10:21, titzer wrote:
> I don't understand why this is a SKIP_WRITE_BARRIER and then a RecordSlot
directly afterwards. Would a full barrier just do the right thing?

We skip the write barrier because this code is executed from the GC, not the
mutator and GC does not execute write barriers. Ulan, is that correct?

[ulan, 2016-06-09 14:45:22]

looking good

https://codereview.chromium.org/2045263002/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2045263002/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:5735: if (rinfo->IsInConstantPool()) {
Could you please extract this code into a function and share with mark compact?

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

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1566: collector_->heap()->RecordWriteIntoCode(host,
rinfo, target);
Replace it with a comment that we don't need to record old->new slot because
target is always in old space.

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1575: collector_->heap()->RecordWriteIntoCode(host,
rinfo, target);
ditto

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1591: collector_->heap()->RecordWriteIntoCode(host,
rinfo, cell);
ditto

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2472: list->Set(counter, obj, SKIP_WRITE_BARRIER);
On 2016/06/09 11:20:40, ahaas wrote:
> On 2016/06/09 at 11:10:21, titzer wrote:
> > I don't understand why this is a SKIP_WRITE_BARRIER and then a RecordSlot
> directly afterwards. Would a full barrier just do the right thing?
> 
> We skip the write barrier because this code is executed from the GC, not the
> mutator and GC does not execute write barriers. Ulan, is that correct?

Yes, the barrier will bailout because marking is finished at this point, so we
need to manually record slot here.

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:4015: heap()->RecordWriteIntoCode(host, &rinfo,
target);
ditto

[ahaas, 2016-06-10 11:00:34]

https://codereview.chromium.org/2045263002/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2045263002/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:5735: if (rinfo->IsInConstantPool()) {
On 2016/06/09 at 14:45:22, ulan wrote:
> Could you please extract this code into a function and share with mark
compact?

As discussed in person sharing this code and still inlining it is difficult
because RelocInfo::IsCodeTarget(rmode) requires to include inline headers.

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

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1566: collector_->heap()->RecordWriteIntoCode(host,
rinfo, target);
On 2016/06/09 at 14:45:22, ulan wrote:
> Replace it with a comment that we don't need to record old->new slot because
target is always in old space.

done.

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1575: collector_->heap()->RecordWriteIntoCode(host,
rinfo, target);
On 2016/06/09 at 14:45:22, ulan wrote:
> ditto

done.

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1591: collector_->heap()->RecordWriteIntoCode(host,
rinfo, cell);
On 2016/06/09 at 14:45:22, ulan wrote:
> ditto

done.

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:4015: heap()->RecordWriteIntoCode(host, &rinfo,
target);
On 2016/06/09 at 14:45:22, ulan wrote:
> ditto

done.

[ulan, 2016-06-10 11:09:34]

LGTM!

What performance numbers look like?

https://codereview.chromium.org/2045263002/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2045263002/diff/60001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1565: // target is always in old space, we don't have
to record the slot in the
Nit: since it is a sentence, let's start it with an upper case letter here and
below.

[titzer, 2016-06-10 13:27:06]

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

https://codereview.chromium.org/2045263002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2472: list->Set(counter, obj, SKIP_WRITE_BARRIER);
On 2016/06/09 14:45:22, ulan wrote:
> On 2016/06/09 11:20:40, ahaas wrote:
> > On 2016/06/09 at 11:10:21, titzer wrote:
> > > I don't understand why this is a SKIP_WRITE_BARRIER and then a RecordSlot
> > directly afterwards. Would a full barrier just do the right thing?
> > 
> > We skip the write barrier because this code is executed from the GC, not the
> > mutator and GC does not execute write barriers. Ulan, is that correct?
> 
> Yes, the barrier will bailout because marking is finished at this point, so we
> need to manually record slot here.

Let's add a comment to document this.

[titzer, 2016-06-13 09:06:04]

lgtm

[ahaas, 2016-06-21 10:25:56]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-06-21 10:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2045263002/#ps120001
(title: "rebase.")

[commit-bot: I haz the power, 2016-06-21 10:26:04]

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

[commit-bot: I haz the power, 2016-06-21 10:40:24]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-21 10:40:59]

Description was changed from

==========
[heap] Avoid the use of cells to point from code to new-space objects.

Cells were needed originally because there was no typed remembered set to
record direct pointers from code space to new space. A previous
CL (https://codereview.chromium.org/2003553002/) already introduced
the remembered set, this CL uses it.

This CL
* stores direct pointers in code objects, even if the target is in new space,
* records the slot of the pointer in typed-old-to-new remembered set,
* adds a list which stores weak code-to-new-space references,
* adds a test to test-heap.cc for weak code-to-new-space references,
* removes prints in tail-call-megatest.js

R=ulan@chromium.org
==========

to

==========
[heap] Avoid the use of cells to point from code to new-space objects.

Cells were needed originally because there was no typed remembered set to
record direct pointers from code space to new space. A previous
CL (https://codereview.chromium.org/2003553002/) already introduced
the remembered set, this CL uses it.

This CL
* stores direct pointers in code objects, even if the target is in new space,
* records the slot of the pointer in typed-old-to-new remembered set,
* adds a list which stores weak code-to-new-space references,
* adds a test to test-heap.cc for weak code-to-new-space references,
* removes prints in tail-call-megatest.js

R=ulan@chromium.org

Committed: https://crrev.com/2d2087b79a293a92a6ed34a2775e481ff2173b3c
Cr-Commit-Position: refs/heads/master@{#37134}
==========

[commit-bot: I haz the power, 2016-06-21 10:41:00]

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

[Michael Achenbach, 2016-06-21 12:09:10]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2087463004/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks arm debug:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm%20....

[ahaas, 2016-06-21 13:09:10]

On 2016/06/21 at 12:09:10, machenbach wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2087463004/ by machenbach@chromium.org.
> 
> The reason for reverting is: [Sheriff] Breaks arm debug:
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm%20....

@v8-mips-ports PTAL. Is it okay that I use the LiFlags parameter also when the
value is not a smi? I want to get rid of the IsSmi check here because it causes
problems in some benchmarks.

[Marija Antic, 2016-06-23 10:43:02]

On 2016/06/21 13:09:10, ahaas wrote:
> On 2016/06/21 at 12:09:10, machenbach wrote:
> > A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2087463004/ by mailto:machenbach@chromium.org.
> > 
> > The reason for reverting is: [Sheriff] Breaks arm debug:
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm%20....
> 
> @v8-mips-ports PTAL. Is it okay that I use the LiFlags parameter also when the
> value is not a smi? I want to get rid of the IsSmi check here because it
causes
> problems in some benchmarks.

lgtm