Thanks for the review Mads! I fixed glitches and I am going to commit this change.
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(), On 2010/04/22 13:19:29, Mads Ager wrote:
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 Done. It seems there is a bug/inconsistency in the emacs indenter. Apparently it thinks that ProcessNonLive is function we are calling. 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 On 2010/04/22 13:19:29, Mads Ager wrote:
it to -> them on the?
Done. 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); On 2010/04/22 13:19:29, Mads Ager wrote:
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?
Indeed! Thanks for spotting this. This loop is awfully complicated as was rewritten several times so I missed it. http://codereview.chromium.org/1683001/diff/1/8#newcode1358 src/mark-compact.cc:1358: // We have to deallocate and put into freelist ending area On 2010/04/22 13:19:29, Mads Ager wrote:
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?
Done. http://codereview.chromium.org/1683001/diff/1/8#newcode1382 src/mark-compact.cc:1382: // to the beggining of first empty page. On 2010/04/22 13:19:29, Mads Ager wrote:
beggining -> beginning.
Done. http://codereview.chromium.org/1683001/diff/1/8#newcode1395 src/mark-compact.cc:1395: if (first_empty_page == NULL) { On 2010/04/22 13:19:29, Mads Ager wrote:
Combine all of this into one ASSERT or use #ifdef DEBUG?
Done. 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()) On 2010/04/22 13:19:29, Mads Ager wrote:
Please use braces around the body of a multi-line if statement.
Done. http://codereview.chromium.org/1683001/diff/1/6#newcode683 src/spaces.cc:683: if (p->WasInUseBeforeMC()) On 2010/04/22 13:19:29, Mads Ager wrote:
Ditto.
Done. http://codereview.chromium.org/1683001/diff/1/6#newcode691 src/spaces.cc:691: if (last_page->WasInUseBeforeMC()) On 2010/04/22 13:19:29, Mads Ager wrote:
Ditto.
Done. http://codereview.chromium.org/1683001/diff/1/6#newcode755 src/spaces.cc:755: pages_list_is_chunk_ordered_ = true; On 2010/04/22 13:19:29, Mads Ager wrote:
pages_list -> page_list
Done. 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; On 2010/04/22 13:19:29, Mads Ager wrote:
Is this just a bit field? If so, why use an intptr_t here instead of
an int? It's legacy. I used this field to link pages into auxiliary list. Removed. 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 On 2010/04/22 13:19:29, Mads Ager wrote:
the different chunks -> different chunks. p is last -> p is the last.
Done. http://codereview.chromium.org/1683001/diff/1/5#newcode550 src/spaces.h:550: static void RelinkPagesListInChunkOrder(PagedSpace* space, On 2010/04/22 13:19:29, Mads Ager wrote:
PagesList -> PageList
Done. http://codereview.chromium.org/1683001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
