Weak Cells

Weak Cells

Introduce an object that holds a weak reference.
Design document: http://goo.gl/9dSvvy.

BUG=
R=erik.corry@gmail.com

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

[Erik Corry, 2014-10-13 15:56:16]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2014-10-13 15:56:17]

LGTM

https://codereview.chromium.org/640303006/diff/180001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/640303006/diff/180001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2702: WeakCell::cast(result)->update_value_from_gc(value);
This is from allocation, not from GC, so probably should not call this function.

https://codereview.chromium.org/640303006/diff/180001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2703: WeakCell::cast(result)->set_next(undefined_value());
There's no need to set the write barrier when setting to undefined, is there? I
think it's immovable and immortal.  So we could add SKIP_WRITE_BARRIER here.

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

https://codereview.chromium.org/640303006/diff/180001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2748: weak_cell->set_next(undefined);
No write barrier needed.

https://codereview.chromium.org/640303006/diff/180001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2760: weak_cell->set_next(undefined);
No write barrier needed

https://codereview.chromium.org/640303006/diff/180001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2777: // We scavange new space simultaneously with
sweeping. This is done in two
scavange -> scavenge

https://codereview.chromium.org/640303006/diff/180001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/640303006/diff/180001/src/objects-inl.h#newco...
src/objects-inl.h:1940: WRITE_BARRIER(GetHeap(), this, kValueOffset, val);
If we are in GC, do we need the write barrier?  I think this is only
legitimately used to set the value to 'undefined', so pehaps it should be
renamed clear_value_from_gc and it should not take an argument.

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

https://codereview.chromium.org/640303006/diff/180001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:4262: heap->CollectAllAvailableGarbage();
The tests should also test that the weak cell is unchanged if there is a full GC
and the value is also reachable through normal objects.

[ulan, 2014-10-14 10:03:42]

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

[ulan, 2014-10-14 10:17:23]

Thanks for review, Erik. I uploaded new PS and add a link to design doc.

I also changed the sentinel end-of-list value to Smi::FromInt(0) from undefined
to make the following invariant:

(!W->value()->IsUndefined()) == <W is in the list>.

https://codereview.chromium.org/640303006/diff/180001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/640303006/diff/180001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2702: WeakCell::cast(result)->update_value_from_gc(value);
On 2014/10/13 15:56:17, Erik Corry wrote:
> This is from allocation, not from GC, so probably should not call this
function.

Introduced a new function "initialize" for this and renamed
"update_value_from_gc" to "clear".

https://codereview.chromium.org/640303006/diff/180001/src/heap/heap.cc#newcod...
src/heap/heap.cc:2703: WeakCell::cast(result)->set_next(undefined_value());
On 2014/10/13 15:56:17, Erik Corry wrote:
> There's no need to set the write barrier when setting to undefined, is there?
I
> think it's immovable and immortal.  So we could add SKIP_WRITE_BARRIER here.

Done.

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

https://codereview.chromium.org/640303006/diff/180001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2748: weak_cell->set_next(undefined);
On 2014/10/13 15:56:17, Erik Corry wrote:
> No write barrier needed.

Done.

https://codereview.chromium.org/640303006/diff/180001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2760: weak_cell->set_next(undefined);
On 2014/10/13 15:56:17, Erik Corry wrote:
> No write barrier needed

Done.

https://codereview.chromium.org/640303006/diff/180001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2777: // We scavange new space simultaneously with
sweeping. This is done in two
On 2014/10/13 15:56:17, Erik Corry wrote:
> scavange -> scavenge

Done.

https://codereview.chromium.org/640303006/diff/180001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/640303006/diff/180001/src/objects-inl.h#newco...
src/objects-inl.h:1940: WRITE_BARRIER(GetHeap(), this, kValueOffset, val);
On 2014/10/13 15:56:17, Erik Corry wrote:
> If we are in GC, do we need the write barrier?  I think this is only
> legitimately used to set the value to 'undefined', so pehaps it should be
> renamed clear_value_from_gc and it should not take an argument.

Yes, we need write barrier for the first initialization. All other usages are
indeed "set to undefined" that don't need write barrier. I introduced two
functions: initialize and clear.

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

https://codereview.chromium.org/640303006/diff/180001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:4262: heap->CollectAllAvailableGarbage();
On 2014/10/13 15:56:17, Erik Corry wrote:
> The tests should also test that the weak cell is unchanged if there is a full
GC
> and the value is also reachable through normal objects.

Done.

[ulan, 2014-10-14 13:22:46]

Michael, Hannes: this is fyi.

[Erik Corry, 2014-10-14 13:37:22]

LGTM with comments

https://codereview.chromium.org/640303006/diff/330001/src/heap/objects-visiti...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/640303006/diff/330001/src/heap/objects-visiti...
src/heap/objects-visiting-inl.h:360: if (weak_cell->next() ==
heap->undefined_value()) {
I think this should set to smi zero.

Since there's no set_value function, there's no need to chain up the weak cell
if it only contains undefined.

Even if there was a set_value method (is there actually one? the design doc says
there isn't...) the write barrier should ensure that we come back here if it is
used.

[ulan, 2014-10-14 14:05:41]

https://codereview.chromium.org/640303006/diff/330001/src/heap/objects-visiti...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/640303006/diff/330001/src/heap/objects-visiti...
src/heap/objects-visiting-inl.h:360: if (weak_cell->next() ==
heap->undefined_value()) {
On 2014/10/14 13:37:22, Erik Corry wrote:
> I think this should set to smi zero.
> 
> Since there's no set_value function, there's no need to chain up the weak cell
> if it only contains undefined.
> 
> Even if there was a set_value method (is there actually one? the design doc
says
> there isn't...) the write barrier should ensure that we come back here if it
is
> used.

Good idea about skipping cleared weak cells. Added a check.

About setting to smi zero: undefined and smi zero help us distinguish the
following cases:
- weak cell is not in the list (next() == undefined)
- weak cell is the last element of the list (next() == smi zero)
- weak cell is in the list and not the last element (next() == pointer to a weak
cell)

We could probably use smi zero for first two cases, but then it would complicate
debugging.

[ulan, 2014-10-14 14:44:06]

Committed patchset #8 (id:350001) manually as 24606 (presubmit successful).