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: // We are sweeping code and map spaces presisely so clearing is not required. presisely -> precisely, 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; Variable should be called leading_zeros http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode1819 src/mark-compact.cc:1819: uint32_t cell = Page::kFirstUsedCell; See above. http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode1820 src/mark-compact.cc:1820: uint32_t poluted_cell = Page::kFirstUsedCell; poluted -> polluted 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) { Lint? 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 -> or http://codereview.chromium.org/6321008/diff/1/src/mark-compact.cc#newcode2043 src/mark-compact.cc:2043: // Implement specialized sweeper for map space. Missing TODO? 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); We will need to make this work on non-gcc at some point. http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode123 src/mark-compact.h:123: Page* p = Page::FromAddress(start); can we call this variable page and the b variable bitmap? http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode125 src/mark-compact.h:125: // Space above linearity boundary is continious. continious -> continuous http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode126 src/mark-compact.h:126: if (start >= p->linearity_boundary()) return start; how do we know an object starts here? This should be mentioned in the documentation of the function. http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode129 src/mark-compact.h:129: uint32_t markbit = p->Address2Markbit(start); 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. http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode140 src/mark-compact.h:140: p->Address2Markbit(limit))); 'To' is ! much longer than '2' and more clear2read&grok! http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode142 src/mark-compact.h:142: ASSERT(cell < last_cell); Comm /* comment */ ent? http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode167 src/mark-compact.h:167: uint32_t cell = Page::MarkbitsBitmap::Index2Cell(markbit); Again, it's an int. Can we call it cell_index to indicate it's not the actual cell? http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode180 src/mark-compact.h:180: uint32_t last_cell = This is actually one past the last 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) || IS_CONTINIOUS -> IS_CONTINUOUS http://codereview.chromium.org/6321008/diff/1/src/mark-compact.h#newcode500 src/mark-compact.h:500: enum SweeperType { CONSERVATIVE, PRECISE }; The style guide allows kConservative and kPrecise. I prefer them, you don't? 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. How are they cleaned? 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#newcode616 src/spaces.h:616: Address linearity_boundary_; Comment? 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 I can't quite parse this. http://codereview.chromium.org/6321008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
