Implement progress bar for large objects. (issue 11362246)

[mstar...@chromium.org]

Reviewers: ulan, hpayer_chromium.org,

Description:
Implement progress bar for large objects.

This implements incremental scanning of large objects using a progress
bar in the page header of such objects. Note that this requires forward
white to gray transitions in the write barrier and hence is disabled by
default for now.

R=ul...@chromium.org,hpa...@chromium.org


Please review this at https://codereview.chromium.org/11362246/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/flag-definitions.h
M src/incremental-marking.h
M src/incremental-marking.cc
M src/mark-compact.cc
M src/objects-visiting-inl.h
M src/objects-visiting.h
M src/spaces.h
M src/spaces.cc

[hpa...@chromium.org]

https://codereview.chromium.org/11362246/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/11362246/diff/1/src/flag-definitions.h#newcode441
src/flag-definitions.h:441: DEFINE_bool(use_progress_bar, false,
should we mention large object in the parameter?
large_object_pogress_bar?

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

https://codereview.chromium.org/11362246/diff/1/src/incremental-marking.cc#newcode228
src/incremental-marking.cc:228: Heap* heap = map->GetHeap();
can you set that flag earlier e.g. at allocation time?

https://codereview.chromium.org/11362246/diff/1/src/incremental-marking.cc#newcode247
src/incremental-marking.cc:247: if (end_offset < object_size) {
why is this method called UnshiftGrey and not Unshift (or enqueue)?
Wouldn't it be better to push the object on the marking deque before we
pop the referenced objects? Doing that will keep the object closer to
its referenced objects.

https://codereview.chromium.org/11362246/diff/1/src/incremental-marking.cc#newcode305
src/incremental-marking.cc:305: // Marks the object grey and pushes it
on the marking stack.
update comment

https://codereview.chromium.org/11362246/

[ul...@chromium.org]

LGTM with few nits.


https://chromiumcodereview.appspot.com/11362246/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/flag-definitions.h#newcode441
src/flag-definitions.h:441: DEFINE_bool(use_progress_bar, false,

On 2012/11/14 12:21:04, hpayer1 wrote:
> should we mention large object in the parameter?
large_object_pogress_bar?

Agreed, "progress bar" sounds to ambiguous in flags. Maybe
"marking_progress_bar"?

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode237
src/incremental-marking.cc:237: int start_offset =
Max(FixedArray::BodyDescriptor::kStartOffset,
Not sure if worthwhile as the loop rarely executes more than once: we
could move the initialization of start_offset and end_offset out of the
loop, and at the end of loop do: start_offset = end_offset, end_offset =
Min(object_size, end_offset + kProgressBarScanningChunk);
This would allow to get rid of chunk->progress_bar() accesses in the
loop and do chunk->set_progress_bar(end_offset) once after the loop.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode240
src/incremental-marking.cc:240: chunk->progress_bar() +
kProgressBarScanningChunk);
Shouldn't this be start_offset + kProgressBarScanningChunk ?

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode247

src/incremental-marking.cc:247: if (end_offset < object_size) {

(end_offset < object_size && !scan_until_end) would be more clear here.

https://chromiumcodereview.appspot.com/11362246/

[mstar...@chromium.org]

Addressed comments.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/flag-definitions.h#newcode441
src/flag-definitions.h:441: DEFINE_bool(use_progress_bar, false,

On 2012/11/15 09:54:23, ulan wrote:
> On 2012/11/14 12:21:04, hpayer1 wrote:
> > should we mention large object in the parameter?
large_object_pogress_bar?

> Agreed, "progress bar" sounds to ambiguous in flags. Maybe
> "marking_progress_bar"?

Done.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode228

src/incremental-marking.cc:228: Heap* heap = map->GetHeap();

On 2012/11/14 12:21:04, hpayer1 wrote:

> can you set that flag earlier e.g. at allocation time?

Yes, that would be cleaner. I just couldn't find a clean bottle-neck in
the allocation function. I'll try to find one.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode237
src/incremental-marking.cc:237: int start_offset =
Max(FixedArray::BodyDescriptor::kStartOffset,

On 2012/11/15 09:54:23, ulan wrote:
> Not sure if worthwhile as the loop rarely executes more than once: we
could move
> the initialization of start_offset and end_offset out of the loop, and
at the
> end of loop do: start_offset = end_offset, end_offset =
Min(object_size,
> end_offset + kProgressBarScanningChunk);
> This would allow to get rid of chunk->progress_bar() accesses in the
loop and do
> chunk->set_progress_bar(end_offset) once after the loop.

Done.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode240
src/incremental-marking.cc:240: chunk->progress_bar() +
kProgressBarScanningChunk);

On 2012/11/15 09:54:23, ulan wrote:
> Shouldn't this be start_offset + kProgressBarScanningChunk ?

Done.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode247
src/incremental-marking.cc:247: if (end_offset < object_size) {

On 2012/11/15 09:54:23, ulan wrote:
> (end_offset < object_size && !scan_until_end) would be more clear
here.

Done.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode247
src/incremental-marking.cc:247: if (end_offset < object_size) {

On 2012/11/14 12:21:04, hpayer1 wrote:

> why is this method called UnshiftGrey and not Unshift (or enqueue)?
> Wouldn't it be better to push the object on the marking deque before
we pop the
> referenced objects? Doing that will keep the object closer to its
referenced
> objects.

The method is called UnshiftGrey because at the time the object is
pushed, it is supposed to be grey, which also holds in this case. If you
call UnshiftBlack it will do a black-to-grey transition if the marking
deque is full to make sure it gets added again once the marking deque is
refilled.

I think pushing will not reduce pressure on the deque, because the large
array would stay in front of all reference objects all the time and the
pressure introduced by the references objects will not reduce until the
large array is fully scanned. So I think we should try to keep the large
array at the end of the marking deque. WDYT?

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode305

src/incremental-marking.cc:305: // Marks the object grey and pushes it
on the marking stack.

On 2012/11/14 12:21:04, hpayer1 wrote:

> update comment

I think the comment still applies. I didn't change semantics of this,
just moved the incrementing of live-bytes into the helper method. The
object is only marked black if it's a data objects and that is just an
optimization.

https://chromiumcodereview.appspot.com/11362246/

[hpa...@chromium.org]

LGTM

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode247
src/incremental-marking.cc:247: if (end_offset < object_size) {

There is probably no performance difference, I just found it cleaner to
have a stack access pattern.

[mstar...@chromium.org]

Landing.

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-marking.cc#newcode228
src/incremental-marking.cc:228: Heap* heap = map->GetHeap();

On 2012/11/14 12:21:04, hpayer1 wrote:

> can you set that flag earlier e.g. at allocation time?

Left a TODO in the code about that. Currently there are just too many
bottle-necks that allocate a fixed array.

https://chromiumcodereview.appspot.com/11362246/