LGTM
http://codereview.chromium.org/6088012/diff/3001/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/6088012/diff/3001/src/heap.cc#newcode218 src/heap.cc:218: int Heap::GcSafeSizeOfOldObject(HeapObject* object) { Shouldn't we just get rid of this alltogether? http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc File src/mark-compact.cc (right): http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode111 src/mark-compact.cc:111: // Check that swept all marked objects and that swept -> that we swept Also formatting is off. http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode543 src/mark-compact.cc:543: ASSERT(!Marking::IsMarked(obj->address())); I wonder whether the compiler can work out that the -1 that the ->address() performs can be omitted because the IsMarked shifts away the low bits. http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode554 src/mark-compact.cc:554: static inline bool VisitUnmarkedObjects(Object** start, Object** end) { It would be nice to disassemble this function to see whether the heroic inlining we are expecting of the C++ compiler is actually taking place http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode691 src/mark-compact.cc:691: static inline Map* SafeMap(Object* obj) { Get rid of this function? http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode1239 src/mark-compact.cc:1239: // Because the object is marked, we have to recover the original map comment out of date http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode1421 src/mark-compact.cc:1421: bool MarkCompactCollector::SafeIsMap(HeapObject* object) { Remove or rename? http://codereview.chromium.org/6088012/diff/3001/src/mark-compact.cc#newcode1948 src/mark-compact.cc:1948: int MarkCompactCollector::SizeOfMarkedObject(HeapObject* obj) { This function is not needed http://codereview.chromium.org/6088012/diff/3001/src/spaces.h File src/spaces.h (right): http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode117 src/spaces.h:117: // Bitmap is a sequence of cells each containing fixed number of bits. Perhaps add a comment with the word 'endian' since this stuff looks endian-dependent. Or do we only ever access in words, never in bytes? http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode168 src/spaces.h:168: INLINE(void Set(uint32_t index, bool value)) { I think it's nicer to have Set and Clear methods instead of relying on the inliner to do that for you. http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode182 src/spaces.h:182: const uint32_t start_mask = (-1) << start; I think you are assuming here that the rhs of the shift operand will be masked down, but it would be clearer and less fragile if you did the mask explicitly. http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode189 src/spaces.h:189: cells()[end_cell] &= ~end_mask; I think this reads and writes (but doesn't change) the word after the end if start+size hits a word boundary. http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode199 src/spaces.h:199: Why two blank lines? http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode280 src/spaces.h:280: static const size_t kBitsPerByteLog2 = 3; This constant already exists in globals.h http://codereview.chromium.org/6088012/diff/3001/src/spaces.h#newcode313 src/spaces.h:313: // TODO [EVE] when do we need this crap? Does this lint or pass the FCC? http://codereview.chromium.org/6088012/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
