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. http://codereview.chromium.org/6321008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
