Implement map collection for incremental marking.

Implement map collection for incremental marking.

This causes map transitions to be treated weakly during incremental
marking and hence allows clearing of non-live transitions. The marking
code is now shared between incremental and non-incremental mode.

R=vegorov@chromium.org
BUG=v8:1465
TEST=cctest/test-heap/Regress1465

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

[Vyacheslav Egorov (Chromium), 2012-05-11 12:58:35]

Round of comments.

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-markin...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-markin...
src/incremental-marking.cc:660: Map* map = obj->map();
Hurry should also treat maps in a special way.

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-markin...
src/incremental-marking.cc:827: } else if (map->instance_type() == JS_MAP_TYPE)
{
s/JS_MAP_TYPE/MAP_TYPE/

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
File src/incremental-marking-inl.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
src/incremental-marking-inl.h:131: bool
IncrementalMarking::MarkObject(HeapObject* obj) {
I suggest naming it MarkObjectAndPush

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
src/incremental-marking-inl.h:145: MarkBlackOrKeepGrey(mark_bit);
Can it be grey at this point? I don't think so.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
src/incremental-marking.cc:836: if (FLAG_collect_maps && map->instance_type() >=
FIRST_JS_RECEIVER_TYPE) {
long line

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact-inl.h
File src/mark-compact-inl.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact-in...
src/mark-compact-inl.h:55: bool MarkCompactCollector::MarkObject(HeapObject*
obj) {
Usually we avoid to use function overloading (per style guide).

In this case it's especially dangerous as MarkObject(HeapObject*) and
MarkObject(HeapObject*, MarkBit) do _completely_ different things.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc...
src/mark-compact.cc:286: if (FLAG_collect_maps) ClearNonLiveTransitions();
It seems that running with --nocollect-maps would really cause maps tree to be
retained in both directions (from leaves to root and from root to leaves).

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc...
src/mark-compact.cc:665: clear_map_transitions_ = true;
Why does not it depend on FLAG_collect_maps?

Flag and this field have some very strange relation.

Having something that is always true does not help.

If you change it to false then potentially you might get broken heap structure
(back pointer from live map pointing to a dead map). So for now I suggest we
should either remove it or make it work.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc...
src/mark-compact.cc:1808: // Mark the Object* fields of the Map. Since the
descriptor array has been
Try to move this to MarkMapContents to further share code?

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.h
File src/mark-compact.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.h#...
src/mark-compact.h:404: MarkCompactCollector* mark_compact_collector() {
empty line before definition

[Michael Starzinger, 2012-05-11 14:51:53]

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-markin...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-markin...
src/incremental-marking.cc:660: Map* map = obj->map();
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> Hurry should also treat maps in a special way.

Done.

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-markin...
src/incremental-marking.cc:827: } else if (map->instance_type() == JS_MAP_TYPE)
{
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> s/JS_MAP_TYPE/MAP_TYPE/

Already fixed in second patch set.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
File src/incremental-marking-inl.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
src/incremental-marking-inl.h:131: bool
IncrementalMarking::MarkObject(HeapObject* obj) {
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> I suggest naming it MarkObjectAndPush

Done.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
src/incremental-marking-inl.h:145: MarkBlackOrKeepGrey(mark_bit);
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> Can it be grey at this point? I don't think so.

Done, inlined mark bit setting.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-mar...
src/incremental-marking.cc:836: if (FLAG_collect_maps && map->instance_type() >=
FIRST_JS_RECEIVER_TYPE) {
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> long line

Done.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact-inl.h
File src/mark-compact-inl.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact-in...
src/mark-compact-inl.h:55: bool MarkCompactCollector::MarkObject(HeapObject*
obj) {
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> Usually we avoid to use function overloading (per style guide).
> 
> In this case it's especially dangerous as MarkObject(HeapObject*) and
> MarkObject(HeapObject*, MarkBit) do _completely_ different things.

Done, renamed to MarkObjectAndPush().

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc...
src/mark-compact.cc:286: if (FLAG_collect_maps) ClearNonLiveTransitions();
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> It seems that running with --nocollect-maps would really cause maps tree to be
> retained in both directions (from leaves to root and from root to leaves).

Yes, now the flag treats pointers in both direction (i.e. "up the tree" and
"down the tree") as strong. But IMHO that is what the name of that flag
suggests.

You are right, we still need to figure out how to collect whole map trees when
their roots are unreachable. But that should be done without a flag.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc...
src/mark-compact.cc:665: clear_map_transitions_ = true;
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> Why does not it depend on FLAG_collect_maps?
> 
> Flag and this field have some very strange relation.
> 
> Having something that is always true does not help.
> 
> If you change it to false then potentially you might get broken heap structure
> (back pointer from live map pointing to a dead map). So for now I suggest we
> should either remove it or make it work.

Done, removed flag for now.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.cc...
src/mark-compact.cc:1808: // Mark the Object* fields of the Map. Since the
descriptor array has been
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> Try to move this to MarkMapContents to further share code?

Done.

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.h
File src/mark-compact.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/mark-compact.h#...
src/mark-compact.h:404: MarkCompactCollector* mark_compact_collector() {
On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> empty line before definition

Done.

[Vyacheslav Egorov (Chromium), 2012-05-14 20:53:41]

LGTM

I recommend passing test with FLAG_collect_maps forces to false on all
architectures before committing.

[Michael Starzinger, 2012-05-15 08:17:20]

On 2012/05/14 20:53:41, Vyacheslav Egorov wrote:
> LGTM
> 
> I recommend passing test with FLAG_collect_maps forces to false on all
> architectures before committing.

If you run the test suite with FLAG_collect_maps set to false by default, you
only get the obvious failures:

* cctest/test-heap/LeakGlobalContextViaMapProto
* cctest/test-heap/PrototypeTransitionClearing
* cctest/test-heap/Regress1465

I don't think it makes sense to mask away these failures when map collection is
disabled, since they perfectly indicate what goes wrong when you disable it.
Besides the above failures, the test suite runs through (on ia32, x64 and ARM).