Implement map collection for incremental marking. (issue 10386046)

[mstar...@chromium.org]

Reviewers: Vyacheslav Egorov,

Description:
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=veg...@chromium.org
BUG=v8:1465

Please review this at https://chromiumcodereview.appspot.com/10386046/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/incremental-marking-inl.h
M src/incremental-marking.h
M src/incremental-marking.cc
M src/mark-compact-inl.h
M src/mark-compact.h
M src/mark-compact.cc

[veg...@chromium.org]

Round of comments.


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

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-marking.cc#newcode660
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-marking.cc#newcode827
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-marking-inl.h
File src/incremental-marking-inl.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-marking-inl.h#newcode131
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-marking-inl.h#newcode145
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-marking.cc
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-marking.cc#newcode836
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-inl.h#newcode55
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#newcode286
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#newcode665
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#newcode1808
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#newcode404
src/mark-compact.h:404: MarkCompactCollector* mark_compact_collector() {
empty line before definition

https://chromiumcodereview.appspot.com/10386046/

[mstar...@chromium.org]

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

https://chromiumcodereview.appspot.com/10386046/diff/1/src/incremental-marking.cc#newcode660
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-marking.cc#newcode827
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-marking-inl.h
File src/incremental-marking-inl.h (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-marking-inl.h#newcode131
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-marking-inl.h#newcode145
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-marking.cc
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/10386046/diff/2001/src/incremental-marking.cc#newcode836
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-inl.h#newcode55
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#newcode286
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#newcode665
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#newcode1808
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#newcode404
src/mark-compact.h:404: MarkCompactCollector* mark_compact_collector() {

On 2012/05/11 12:58:35, Vyacheslav Egorov wrote:
> empty line before definition

Done.

https://chromiumcodereview.appspot.com/10386046/