Unify processing of weak lists in heap.cc

Unify processing of weak lists in heap.cc

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

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

[Dmitry Lomov (no reviews), 2013-06-07 14:43:41]

Hi Michael _and_ Hannes,

This unifies all our weak list processing.
I have changes array buffer and typed array weak lists to terminate in undefined
instead of Smi 0.

[Michael Starzinger, 2013-06-11 12:59:53]

This is already looking good, I like this change. Just a few comments about the
mechanics of the implementation.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1499
src/heap.cc:1499: Heap* heap,
nit: Indentation is off, also put the "record_slots" parameter on its own line.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1569
src/heap.cc:1569: context->set_unchecked(heap,
I think we can finally switch to use the checked accessor here, this means
'context' will always be a valid heap-object and we don't need to thread the
Heap pointer through set_weak_next.

This is a leftover from the old GC implementation that used in-place marking and
destroyed the map word in object headers. It is tracked by issue 1490.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1633
src/heap.cc:1633: static void set_weak_next(Heap*, JSTypedArray* obj, Object*
next) {
See my previous comment, I think we can drop the Heap pointer argument here and
for the other visitors.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1642
src/heap.cc:1642: Heap* heap,
nit: Can we make the Heap pointer be the first argument to the function? This
would be in line with the other visitors in the code-base.

[Dmitry Lomov (no reviews), 2013-06-11 13:58:43]

On 2013/06/11 12:59:53, Michael Starzinger wrote:
> This is already looking good, I like this change. Just a few comments about
the
> mechanics of the implementation.
> 
> https://codereview.chromium.org/16641003/diff/3001/src/heap.cc
> File src/heap.cc (right):
> 
> https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1499
> src/heap.cc:1499: Heap* heap,
> nit: Indentation is off, also put the "record_slots" parameter on its own
line.
> 
> https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1569
> src/heap.cc:1569: context->set_unchecked(heap,
> I think we can finally switch to use the checked accessor here, this means
> 'context' will always be a valid heap-object and we don't need to thread the
> Heap pointer through set_weak_next.
> 
> This is a leftover from the old GC implementation that used in-place marking
and
> destroyed the map word in object headers. It is tracked by issue 1490.

Excellent, I am happy that Heap parameter can now be removed! Will do.
> 
> https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1633
> src/heap.cc:1633: static void set_weak_next(Heap*, JSTypedArray* obj, Object*
> next) {
> See my previous comment, I think we can drop the Heap pointer argument here
and
> for the other visitors.
> 
> https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1642
> src/heap.cc:1642: Heap* heap,
> nit: Can we make the Heap pointer be the first argument to the function? This
> would be in line with the other visitors in the code-base.

[Dmitry Lomov (no reviews), 2013-06-11 14:47:11]

Comments addressed, also changed names to follow style.
Hannes, could you take a look as well?

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1499
src/heap.cc:1499: Heap* heap,
On 2013/06/11 12:59:53, Michael Starzinger wrote:
> nit: Indentation is off, also put the "record_slots" parameter on its own
line.

Done.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1569
src/heap.cc:1569: context->set_unchecked(heap,
On 2013/06/11 12:59:53, Michael Starzinger wrote:
> I think we can finally switch to use the checked accessor here, this means
> 'context' will always be a valid heap-object and we don't need to thread the
> Heap pointer through set_weak_next.
> 
> This is a leftover from the old GC implementation that used in-place marking
and
> destroyed the map word in object headers. It is tracked by issue 1490.

Done.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1633
src/heap.cc:1633: static void set_weak_next(Heap*, JSTypedArray* obj, Object*
next) {
On 2013/06/11 12:59:53, Michael Starzinger wrote:
> See my previous comment, I think we can drop the Heap pointer argument here
and
> for the other visitors.

Done.

https://codereview.chromium.org/16641003/diff/3001/src/heap.cc#newcode1642
src/heap.cc:1642: Heap* heap,
On 2013/06/11 12:59:53, Michael Starzinger wrote:
> nit: Can we make the Heap pointer be the first argument to the function? This
> would be in line with the other visitors in the code-base.

Done.

[Michael Starzinger, 2013-06-11 14:53:36]

LGTM from my end, it's up to Hannes now.

[Hannes Payer (out of office), 2013-06-12 09:12:08]

LGTM! one comment nit

https://codereview.chromium.org/16641003/diff/11001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/16641003/diff/11001/src/heap.cc#newcode1530
src/heap.cc:1530: // tail is a live object, visit it.
Tail

[Dmitry Lomov (no reviews), 2013-06-12 09:22:59]

https://codereview.chromium.org/16641003/diff/11001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/16641003/diff/11001/src/heap.cc#newcode1530
src/heap.cc:1530: // tail is a live object, visit it.
On 2013/06/12 09:12:08, Hannes Payer wrote:
> Tail
'tail' is a variable name here.

[Dmitry Lomov (no reviews), 2013-06-12 09:31:47]

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