Drain StoreBufferBlock into store buffer instead of iterating over it directly.

Drain StoreBufferBlock into store buffer instead of iterating over it directly.

This will deduplicate its entries. StoreBufferBlock might be contain the same large object dozens of times and without deduplication this will severely degrade GC performance by revisiting the same large object for every time it is recorded in the StoreBufferBlock.

BUG=dart:10048
Committed: https://code.google.com/p/dart/source/detail?r=21766

[Vyacheslav Egorov (Google), 2013-04-19 16:47:01]

Please take a look.

[siva, 2013-04-19 18:27:41]

LGTM with some comments.

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/scavenger.cc#ne...
runtime/vm/scavenger.cc:397: isolate->store_buffer()->ProcessBlock(block);
Why is it not ok to just call
block->ProcessBuffer(isolate);
here.

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/scavenger.cc#ne...
runtime/vm/scavenger.cc:399: block->Reset();
This reset would not be needed if you did a
block->ProcessBuffer(isolate);

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/store_buffer.cc
File runtime/vm/store_buffer.cc (right):

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/store_buffer.cc...
runtime/vm/store_buffer.cc:95: }
This function may not be needed if ProcessBuffer was used and all the other
changes would not be needed.

[Vyacheslav Egorov (Google), 2013-04-19 19:40:20]

Thanks for the review Siva!

Can you please take another look?

Unfortunately I could not use ProcessBuffer as is (explained below) but now I
refactored code to avoid code duplication and have a single place that actually
drains the block.

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/scavenger.cc#ne...
runtime/vm/scavenger.cc:397: isolate->store_buffer()->ProcessBlock(block);
ProcessBuffer would use normal AddPointer and thus it might schedule an
interrupt if we have too many deduplication sets. But we do not actually need
this interrupt --- we are in the middle of scavenge already.

If I would use ProcessBuffer here I might get some redundant scavenges due to
this interrupt.

On 2013/04/19 18:27:41, siva wrote:
> Why is it not ok to just call
> block->ProcessBuffer(isolate);
> here.

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/scavenger.cc#ne...
runtime/vm/scavenger.cc:399: block->Reset();
On 2013/04/19 18:27:41, siva wrote:
> This reset would not be needed if you did a
> block->ProcessBuffer(isolate);

Done.

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/store_buffer.cc
File runtime/vm/store_buffer.cc (right):

https://codereview.chromium.org/14348026/diff/5001/runtime/vm/store_buffer.cc...
runtime/vm/store_buffer.cc:95: }
On 2013/04/19 18:27:41, siva wrote:
> This function may not be needed if ProcessBuffer was used and all the other
> changes would not be needed.

Done.

[siva, 2013-04-19 20:01:08]

lgtm

[Vyacheslav Egorov (Google), 2013-04-19 20:15:23]

Committed patchset #4 manually as r21766 (presubmit successful).