Issue 6321008: Introduce conservative sweeping.

Issue 6321008: Introduce conservative sweeping. (Closed)

Created:
9 years, 11 months ago by Vyacheslav Egorov (Chromium)

Modified:
9 years, 7 months ago

Reviewers:
Erik Corry

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/experimental/gc

Visibility:
Public.

Description

Introduce conservative sweeping. Committed: http://code.google.com/p/v8/source/detail?r=6406

Patch Set 1 #

Total comments: 42

Created: 9 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+465 lines, -84 lines)			Patch
M	src/builtins.cc	View	2 chunks	+4 lines, -0 lines	2 comments	Download
M	src/mark-compact.h	View	2 chunks	+102 lines, -1 line	22 comments	Download
M	src/mark-compact.cc	View	8 chunks	+207 lines, -38 lines	13 comments	Download
M	src/spaces.h	View	14 chunks	+105 lines, -25 lines	3 comments	Download
M	src/spaces.cc	View	10 chunks	+47 lines, -20 lines	2 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Erik Corry

LGTM http://codereview.chromium.org/6321008/diff/1/src/builtins.cc File src/builtins.cc (right): http://codereview.chromium.org/6321008/diff/1/src/builtins.cc#newcode354 src/builtins.cc:354: Marking::TransferMark(elms->address(), Comment? http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc File src/mark-compact.cc (right): http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode149 src/mark-compact.cc:149: ...

9 years, 11 months ago (2011-01-19 13:46:48 UTC) #2

Vyacheslav Egorov (Chromium)

9 years, 11 months ago (2011-01-20 16:40:21 UTC) #3

landed

http://codereview.chromium.org/6321008/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/6321008/diff/1/src/builtins.cc#newcode354
src/builtins.cc:354: Marking::TransferMark(elms->address(),
On 2011/01/19 13:46:48, Erik Corry wrote:
> Comment?

Done.

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

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode149
src/mark-compact.cc:149: // We are sweeping code and map spaces presisely so
clearing is not required.
On 2011/01/19 13:46:48, Erik Corry wrote:
> presisely -> precisely,

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode1798
src/mark-compact.cc:1798: int clz = __builtin_clz(cells[cell - 1]) + 1;
On 2011/01/19 13:46:48, Erik Corry wrote:
> Variable should be called leading_zeros

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode1820
src/mark-compact.cc:1820: uint32_t poluted_cell = Page::kFirstUsedCell;
On 2011/01/19 13:46:48, Erik Corry wrote:
> poluted -> polluted

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode1865
src/mark-compact.cc:1865: static void SweepPrecisely(PagedSpace* space, Page* p,
Address* last_free_start) {
On 2011/01/19 13:46:48, Erik Corry wrote:
> Lint?

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode2042
src/mark-compact.cc:2042: // collection on in one of the previous ones.
On 2011/01/19 13:46:48, Erik Corry wrote:
> on -> or

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode2043
src/mark-compact.cc:2043: // Implement specialized sweeper for map space.
On 2011/01/19 13:46:48, Erik Corry wrote:
> Missing TODO?

Done.

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

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode113
src/mark-compact.h:113: uint32_t bit = __builtin_ctz(cell);
On 2011/01/19 13:46:48, Erik Corry wrote:
> We will need to make this work on non-gcc at some point.

I create separate class for compiler intrinsics.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode123
src/mark-compact.h:123: Page* p = Page::FromAddress(start);
On 2011/01/19 13:46:48, Erik Corry wrote:
> can we call this variable page and the b variable bitmap?

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode125
src/mark-compact.h:125: // Space above linearity boundary is continious.
On 2011/01/19 13:46:48, Erik Corry wrote:
> continious -> continuous

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode126
src/mark-compact.h:126: if (start >= p->linearity_boundary()) return start;
On 2011/01/19 13:46:48, Erik Corry wrote:
> how do we know an object starts here?  This should be mentioned in the
> documentation of the function.

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode129
src/mark-compact.h:129: uint32_t markbit = p->Address2Markbit(start);
On 2011/01/19 13:46:48, Erik Corry wrote:
> I'd prefer int instead of uint32_t.  Firstly unsigned variables cause trouble,
> secondly the explicit specification of the size leads the reader to think it
> matters.

I prefer uint32_t. Having uint32_t there and int here will require some
typecasting here or there.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode140
src/mark-compact.h:140: p->Address2Markbit(limit)));
On 2011/01/19 13:46:48, Erik Corry wrote:
> 'To' is ! much longer than '2' and more clear2read&grok!

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode142
src/mark-compact.h:142: ASSERT(cell < last_cell);
On 2011/01/19 13:46:48, Erik Corry wrote:
> Comm /* comment */ ent?

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode167
src/mark-compact.h:167: uint32_t cell =
Page::MarkbitsBitmap::Index2Cell(markbit);
On 2011/01/19 13:46:48, Erik Corry wrote:
> Again, it's an int.  Can we call it cell_index to indicate it's not the actual
> cell?

Done.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode180
src/mark-compact.h:180: uint32_t last_cell =
On 2011/01/19 13:46:48, Erik Corry wrote:
> This is actually one past the last cell?

Yes. This is frontier cell.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode199
src/mark-compact.h:199:
Page::FromAddress(old_start)->IsFlagSet(Page::IS_CONTINIOUS) ||
On 2011/01/19 13:46:48, Erik Corry wrote:
> IS_CONTINIOUS -> IS_CONTINUOUS

Done everywhere.

http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode500
src/mark-compact.h:500: enum SweeperType { CONSERVATIVE, PRECISE };
On 2011/01/19 13:46:48, Erik Corry wrote:
> The style guide allows kConservative and kPrecise.  I prefer them, you don't?

I prefer kX for non trivial constants.

XXX is for trivial constants.

http://codereview.chromium.org/6321008/diff/1/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/6321008/diff/1/src/spaces.cc#newcode1687
src/spaces.cc:1687: // Just clean them.
On 2011/01/19 13:46:48, Erik Corry wrote:
> How are they cleaned?

Done.

http://codereview.chromium.org/6321008/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/6321008/diff/1/src/spaces.h#newcode926
src/spaces.h:926: // A HeapObjectIterator iterates objects from the bottom of
the given space of
On 2011/01/19 13:46:48, Erik Corry wrote:
> I can't quite parse this.

Done.

Expand Messages | Collapse Messages