LGTM with these comments addressed.
http://codereview.chromium.org/1683001/diff/1/8 File src/mark-compact.cc (right): http://codereview.chromium.org/1683001/diff/1/8#newcode1066 src/mark-compact.cc:1066: p->ObjectAreaStart(), I believe the indentation of these arguments were correct before. When all arguments are on a new line, they should have a four space indent compared to the method name. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls http://codereview.chromium.org/1683001/diff/1/8#newcode1289 src/mark-compact.cc:1289: // of pages without live objects and free them (instead of putting it to free it to -> them on the? http://codereview.chromium.org/1683001/diff/1/8#newcode1346 src/mark-compact.cc:1346: int size_in_bytes = static_cast<int>(p->AllocationTop() - free_start); Isn't there a corner case here where size_in_bytes is incorrect? If the page is empty because nothing is allocated (p->ObjectAreaStart() == p->AllocationTop()) then free_start will be NULL at this point which will make size_in_bytes much bigger than the actual 0 bytes? http://codereview.chromium.org/1683001/diff/1/8#newcode1358 src/mark-compact.cc:1358: // We have to deallocate and put into freelist ending area I think this comment could be a bit clearer. How about something like: If there is a free ending area on one of the previous pages we deallocate that area and put it on the free list? http://codereview.chromium.org/1683001/diff/1/8#newcode1382 src/mark-compact.cc:1382: // to the beggining of first empty page. beggining -> beginning. http://codereview.chromium.org/1683001/diff/1/8#newcode1395 src/mark-compact.cc:1395: if (first_empty_page == NULL) { Combine all of this into one ASSERT or use #ifdef DEBUG? http://codereview.chromium.org/1683001/diff/1/6 File src/spaces.cc (right): http://codereview.chromium.org/1683001/diff/1/6#newcode675 src/spaces.cc:675: if (prev->is_valid()) Please use braces around the body of a multi-line if statement. http://codereview.chromium.org/1683001/diff/1/6#newcode683 src/spaces.cc:683: if (p->WasInUseBeforeMC()) Ditto. http://codereview.chromium.org/1683001/diff/1/6#newcode691 src/spaces.cc:691: if (last_page->WasInUseBeforeMC()) Ditto. http://codereview.chromium.org/1683001/diff/1/6#newcode755 src/spaces.cc:755: pages_list_is_chunk_ordered_ = true; pages_list -> page_list http://codereview.chromium.org/1683001/diff/1/5 File src/spaces.h (right): http://codereview.chromium.org/1683001/diff/1/5#newcode283 src/spaces.h:283: intptr_t flags; Is this just a bit field? If so, why use an intptr_t here instead of an int? http://codereview.chromium.org/1683001/diff/1/5#newcode433 src/spaces.h:433: // and p precedes q in C or p and q belong to the different chunks and p is last the different chunks -> different chunks. p is last -> p is the last. http://codereview.chromium.org/1683001/diff/1/5#newcode550 src/spaces.h:550: static void RelinkPagesListInChunkOrder(PagedSpace* space, PagesList -> PageList http://codereview.chromium.org/1683001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
