Multi-page growing and shrinking new-space

Multi-page growing and shrinking new-space

Committed: http://code.google.com/p/v8/source/detail?r=8282

[Vyacheslav Egorov (Chromium), 2011-06-14 11:52:55]

incremental marking part LGTM

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:162: chunk->SetFlag(MemoryChunk::SCAN_ON_SCAVENGE);
I am not sure this should be here.

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:211: NewSpacePageIterator it(space->ToSpaceLow(),
space->ToSpaceHigh());
I find iterator interface confusing as new space is no longer continuous.

[Erik Corry, 2011-06-14 12:17:17]

LGTM

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc#newcode1006
src/spaces.cc:1006: if (new_limit < allocation_info_.limit) {
This 'if' can be done more elegantly with Min(x,y)

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc#newcode1095
src/spaces.cc:1095: current = page->body();
I think you want a 'continue' here in case the end condition has been reached?

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc#newcode1311
src/spaces.cc:1311: // on from-space pages, so it might be out of sync with the
marking state.
Would it be simpler to update it dynamically on from-space pages rather than
have this 'if'?

http://codereview.chromium.org/7149016/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7149016/diff/1/src/spaces.h#newcode1531
src/spaces.h:1531: static bool at_start(Address addr) {
AtStart() or IsAtStart()

http://codereview.chromium.org/7149016/diff/1/src/spaces.h#newcode1536
src/spaces.h:1536: static bool at_end(Address addr) {
& here

[Erik Corry, 2011-06-14 12:17:43]

no, wait!

[Erik Corry, 2011-06-14 12:23:17]

Now it LGTM

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:162: chunk->SetFlag(MemoryChunk::SCAN_ON_SCAVENGE);
On 2011/06/14 11:52:55, Vyacheslav Egorov wrote:
> I am not sure this should be here.

I think it's correct.  Scan-on-scavenge means that this page will be scanned on
a new-space collection.  This is definitely the case for new-space pages.

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:175: void
IncrementalMarking::DeactivateWriteBarrierForSpace(NewSpace* space) {
This should really be called 'DeactivateIncrementalWriteBarrierForSpace' since
it does not deactivate the write barrier completely, only the remembered set.

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:211: NewSpacePageIterator it(space->ToSpaceLow(),
space->ToSpaceHigh());
On 2011/06/14 11:52:55, Vyacheslav Egorov wrote:
> I find iterator interface confusing as new space is no longer continuous.

Agreed.  It would make more sense if it just took the space as an argument.

[Lasse Reichstein, 2011-06-14 14:13:03]

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:162: chunk->SetFlag(MemoryChunk::SCAN_ON_SCAVENGE);
Keeping it.

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:175: void
IncrementalMarking::DeactivateWriteBarrierForSpace(NewSpace* space) {
search-and-replaced.

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:211: NewSpacePageIterator it(space->ToSpaceLow(),
space->ToSpaceHigh());
Actually that's why I want a page iterator to iterate over the pages of the
semispace without doing the book-keeping every time.

http://codereview.chromium.org/7149016/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:211: NewSpacePageIterator it(space->ToSpaceLow(),
space->ToSpaceHigh());
Changed to use NewSpacePageIterator(space), which does the same thing.

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc#newcode1006
src/spaces.cc:1006: if (new_limit < allocation_info_.limit) {
Done.

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc#newcode1095
src/spaces.cc:1095: current = page->body();
The loop ends immediately after, so that won't make any difference.

http://codereview.chromium.org/7149016/diff/1/src/spaces.cc#newcode1311
src/spaces.cc:1311: // on from-space pages, so it might be out of sync with the
marking state.
Yes, but why move complexity from debug-only code to "real" code.

http://codereview.chromium.org/7149016/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7149016/diff/1/src/spaces.h#newcode1531
src/spaces.h:1531: static bool at_start(Address addr) {
Done

http://codereview.chromium.org/7149016/diff/1/src/spaces.h#newcode1536
src/spaces.h:1536: static bool at_end(Address addr) {
do.