Prototype of mark-and-compact support for Harmony weak maps.

Prototype of mark-and-compact support for Harmony weak maps.

R=vegorov@chromium.org
BUG=v8:1565
TEST=cctest/test-weakmaps

Committed: http://code.google.com/p/v8/source/detail?r=8817

[Vyacheslav Egorov (Chromium), 2011-08-02 16:32:00]

I think it'll LGTM with comments addressed.

http://codereview.chromium.org/7553012/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/7553012/diff/1/src/heap.h#newcode135
src/heap.h:135: V(Object, encountered_weak_maps, EncounteredWeakMaps)           
            \
Do we really need a separate root for this list?

I think just a field on the Heap or MarkCompactCollector should be enough.

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

http://codereview.chromium.org/7553012/diff/1/src/mark-compact.cc#newcode588
src/mark-compact.cc:588:
heap->mark_compact_collector()->SetMark(weak_map->unchecked_table());
What about marking unchecked_table()->map() or at least asserting that it is
already marked (it is a root I presume)

http://codereview.chromium.org/7553012/diff/1/src/mark-compact.cc#newcode1786
src/mark-compact.cc:1786: StaticMarkingVisitor::MarkObjectByPointer(heap(),
&value);
MarkObjectByPointer might modify the value (short circuiting of cons-strings) so
you might want to store it back into table is value was modified.

http://codereview.chromium.org/7553012/diff/1/test/cctest/test-weakmaps.cc
File test/cctest/test-weakmaps.cc (right):

http://codereview.chromium.org/7553012/diff/1/test/cctest/test-weakmaps.cc#ne...
test/cctest/test-weakmaps.cc:108: HEAP->CollectAllGarbage(false);
You can add a check that key is still in the map after first GC.

http://codereview.chromium.org/7553012/diff/1/test/cctest/test-weakmaps.cc#ne...
test/cctest/test-weakmaps.cc:146: //CHECK_EQ(32, weakmap->table()->Capacity());
Instead of leaving commented code (which we do not allow) disable test via
expectations in cctest.status file.

[Michael Starzinger, 2011-08-03 08:47:05]

Uploaded new patch set.

http://codereview.chromium.org/7553012/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/7553012/diff/1/src/heap.h#newcode135
src/heap.h:135: V(Object, encountered_weak_maps, EncounteredWeakMaps)           
            \
On 2011/08/02 16:32:00, Vyacheslav Egorov wrote:
> Do we really need a separate root for this list?
> 
> I think just a field on the Heap or MarkCompactCollector should be enough.

Done. I made it into a root to have the list available for shrinking of tables
after the GC (including a possible compaction) finished. But we do not yet
shrink anyways, so no need for it to be a root.

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

http://codereview.chromium.org/7553012/diff/1/src/mark-compact.cc#newcode588
src/mark-compact.cc:588:
heap->mark_compact_collector()->SetMark(weak_map->unchecked_table());
On 2011/08/02 16:32:00, Vyacheslav Egorov wrote:
> What about marking unchecked_table()->map() or at least asserting that it is
> already marked (it is a root I presume)

Done.

http://codereview.chromium.org/7553012/diff/1/src/mark-compact.cc#newcode1786
src/mark-compact.cc:1786: StaticMarkingVisitor::MarkObjectByPointer(heap(),
&value);
On 2011/08/02 16:32:00, Vyacheslav Egorov wrote:
> MarkObjectByPointer might modify the value (short circuiting of cons-strings)
so
> you might want to store it back into table is value was modified.

Done.

http://codereview.chromium.org/7553012/diff/1/test/cctest/test-weakmaps.cc
File test/cctest/test-weakmaps.cc (right):

http://codereview.chromium.org/7553012/diff/1/test/cctest/test-weakmaps.cc#ne...
test/cctest/test-weakmaps.cc:108: HEAP->CollectAllGarbage(false);
On 2011/08/02 16:32:00, Vyacheslav Egorov wrote:
> You can add a check that key is still in the map after first GC.

Done.

http://codereview.chromium.org/7553012/diff/1/test/cctest/test-weakmaps.cc#ne...
test/cctest/test-weakmaps.cc:146: //CHECK_EQ(32, weakmap->table()->Capacity());
On 2011/08/02 16:32:00, Vyacheslav Egorov wrote:
> Instead of leaving commented code (which we do not allow) disable test via
> expectations in cctest.status file.

Done.

[Vyacheslav Egorov (Chromium), 2011-08-03 08:50:20]

LGTM

http://codereview.chromium.org/7553012/diff/3001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7553012/diff/3001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:119: /*var wm = new WeakMap;
I don't think there is any reason to commit commented code.

[Michael Starzinger, 2011-08-03 12:41:44]

http://codereview.chromium.org/7553012/diff/3001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7553012/diff/3001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:119: /*var wm = new WeakMap;
On 2011/08/03 08:50:20, Vyacheslav Egorov wrote:
> I don't think there is any reason to commit commented code.

Done.