Implement progress bar for large objects.

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=ulan@chromium.org,hpayer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=12978

[Hannes Payer (out of office), 2012-11-14 12:21:04]

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#newcod...
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#ne...
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#ne...
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#ne...
src/incremental-marking.cc:305: // Marks the object grey and pushes it on the
marking stack.
update comment

[ulan, 2012-11-15 09:54:23]

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...
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-markin...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-markin...
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-markin...
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-markin...
src/incremental-marking.cc:247: if (end_offset < object_size) {
(end_offset < object_size && !scan_until_end) would be more clear here.

[Michael Starzinger, 2012-11-15 15:12:50]

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...
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-markin...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-markin...
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-markin...
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-markin...
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-markin...
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-markin...
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-markin...
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.

[Hannes Payer (out of office), 2012-11-15 16:25:54]

LGTM

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

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-markin...
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.

On 2012/11/15 15:12:50, Michael Starzinger wrote:
> 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?

[Michael Starzinger, 2012-11-15 17:54:51]

Landing.

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

https://chromiumcodereview.appspot.com/11362246/diff/1/src/incremental-markin...
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.