- Implement the old sweeper.

- Implement the old sweeper.
Committed: https://code.google.com/p/dart/source/detail?r=2485

[siva, 2011-12-14 16:20:13]

LGTM

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc
File vm/gc_sweeper.cc (right):

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc#newcode10
vm/gc_sweeper.cc:10: #include "vm/object.h"
list vm/object.h before vm/pages.h

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc#newcode18
vm/gc_sweeper.cc:18: intptr_t in_use = page->used();
ASERT(in_use != 0); ?

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.h
File vm/gc_sweeper.h (right):

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.h#newcode25
vm/gc_sweeper.h:25: // all the unmarked onbjects to the freelist.
*onbjects*objects

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.h#newcode32
vm/gc_sweeper.h:32: Heap* heap_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc
File vm/pages.cc (right):

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc#newcode242
vm/pages.cc:242: in_use_ = in_use;
in_use here is not accounting for the large pages in use.

[cshapiro, 2011-12-14 18:48:11]

http://codereview.chromium.org/8898034/diff/1/vm/freelist.h
File vm/freelist.h (right):

http://codereview.chromium.org/8898034/diff/1/vm/freelist.h#newcode36
vm/freelist.h:36: static bool IsSpecialMap(RawObject* raw_obj) {
This is the first time I have seen the class information referred to as a Map. 
That naming is used in V8.  Maybe this should be called IsSpecialClass instead?

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc
File vm/gc_sweeper.cc (right):

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc#newcode38
vm/gc_sweeper.cc:38: uword free_end = current + raw_obj->Size();
How does this interact with FreeList elements?

FreeList objects will not be marked so, if I am following this correctly, if you
sweep a page with a FreeList element between two ordinary objects, you may reach
this case.

Either way, a one-liner comment describing the free storage policy would be
great.

If that is the case, it looks like a FreeList element could be coalesced an the
FreeList would be corrupted.

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc
File vm/pages.cc (right):

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc#newcode247
vm/pages.cc:247: OS::PrintErr("Mark-Sweep[%d]: %lldus (%dK -> %dK, %dK)\n",
Maybe report marking and sweeping time separately?

[Ivan Posva, 2011-12-15 22:29:18]

http://codereview.chromium.org/8898034/diff/1/vm/freelist.h
File vm/freelist.h (right):

http://codereview.chromium.org/8898034/diff/1/vm/freelist.h#newcode36
vm/freelist.h:36: static bool IsSpecialMap(RawObject* raw_obj) {
On 2011/12/14 18:48:11, cshapiro wrote:
> This is the first time I have seen the class information referred to as a Map.

> That naming is used in V8.  Maybe this should be called IsSpecialClass
instead?

Done.

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc
File vm/gc_sweeper.cc (right):

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc#newcode10
vm/gc_sweeper.cc:10: #include "vm/object.h"
On 2011/12/14 16:20:13, asiva wrote:
> list vm/object.h before vm/pages.h

object.h was only needed for testing. Removed.

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc#newcode18
vm/gc_sweeper.cc:18: intptr_t in_use = page->used();
On 2011/12/14 16:20:13, asiva wrote:
> ASERT(in_use != 0); ?

That would be an incorrect ASSERT for totally empty pages. Once we do not even
attempt to sweep entirely empty pages this assert would make sense.

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.cc#newcode38
vm/gc_sweeper.cc:38: uword free_end = current + raw_obj->Size();
On 2011/12/14 18:48:11, cshapiro wrote:
> How does this interact with FreeList elements?
> 
> FreeList objects will not be marked so, if I am following this correctly, if
you
> sweep a page with a FreeList element between two ordinary objects, you may
reach
> this case.
> 
> Either way, a one-liner comment describing the free storage policy would be
> great.
> 
> If that is the case, it looks like a FreeList element could be coalesced an
the
> FreeList would be corrupted.

FreeListElements are coalesced and added to the correct free list. Free lists
are entirely emptied before the sweep cycle.

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.h
File vm/gc_sweeper.h (right):

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.h#newcode25
vm/gc_sweeper.h:25: // all the unmarked onbjects to the freelist.
On 2011/12/14 16:20:13, asiva wrote:
> *onbjects*objects

Done.

http://codereview.chromium.org/8898034/diff/1/vm/gc_sweeper.h#newcode32
vm/gc_sweeper.h:32: Heap* heap_;
On 2011/12/14 16:20:13, asiva wrote:
> DISALLOW_COPY_AND_ASSIGN

DISALLOW_IMPLICIT_CONSTRUCTORS

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc
File vm/pages.cc (right):

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc#newcode242
vm/pages.cc:242: in_use_ = in_use;
On 2011/12/14 16:20:13, asiva wrote:
> in_use here is not accounting for the large pages in use.

Done.

http://codereview.chromium.org/8898034/diff/1/vm/pages.cc#newcode247
vm/pages.cc:247: OS::PrintErr("Mark-Sweep[%d]: %lldus (%dK -> %dK, %dK)\n",
On 2011/12/14 18:48:11, cshapiro wrote:
> Maybe report marking and sweeping time separately?

Many more counters to come here.