On store buffer overflow we mark individidual pages for...

On store buffer overflow we mark individidual pages for
scanning instead of scanning all of old space.

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

[Vyacheslav Egorov (Chromium), 2011-03-28 15:13:19]

LGTM

approach with callback looks a bit overcomplicated though.

http://codereview.chromium.org/6745033/diff/19002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6745033/diff/19002/src/heap.cc#newcode492
src/heap.cc:492: IncrementalMarking::set_should_hurry(false);
I think nicer place to clean the flag is in the IncrementalMarking::Finalize()

http://codereview.chromium.org/6745033/diff/19002/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6745033/diff/19002/src/heap.h#newcode244
src/heap.h:244: // The fullness of the store buffer when we started to scan the
current page.
I can't grok the comment.

http://codereview.chromium.org/6745033/diff/19002/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/6745033/diff/19002/src/mark-compact.cc#newcode...
src/mark-compact.cc:1729:
StoreBuffer::EnterDirectlyIntoStoreBuffer(reinterpret_cast<Address>(p));
This visitor is used to visit new space objects.

Their slots should not be entered into store-buffer.

http://codereview.chromium.org/6745033/diff/19002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6745033/diff/19002/src/objects-inl.h#newcode834
src/objects-inl.h:834: if (Heap::InNewSpace(value)) \
curly brackets around if's body.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode334
src/spaces.h:334: owner_ = reinterpret_cast<Address>(space) + kFailureTag;
add assertion that checks that space was properly aligned.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode449
src/spaces.h:449: // The identity of the owning space.  This is tagged as a heap
pointer, but
Update comment. Now this is tagged as a failure and failures can reside in
objects.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode453
src/spaces.h:453: // This flag indicates that the page is not being tracked by
the store buffer.
Why are we using bool instead of built-in flags?

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode459
src/spaces.h:459: int store_buffer_counter_;
You added 2 fields but kHeaderSize was increased only by 1 kPointerSize.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode2051
src/spaces.h:2051: // FALL THROUGH!
this comment does not look like something we usually write.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc
File src/store-buffer.cc (right):

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode171
src/store-buffer.cc:171: ExemptPopularPages(97, ((Page::kPageSize /
kPointerSize) / 97) / 8);
this looks like a repeatable pattern.
loop with array of prime numbers?

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode231
src/store-buffer.cc:231: containing_chunk =
MemoryChunk::FromAnyPointerAddress(addr);
This code can be pretty slow on certain patterns.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode340
src/store-buffer.cc:340: void StoreBuffer::Verify() {
I think we should have a way to verify store-buffer or we might have some
troubles debugging strange crashes.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode397
src/store-buffer.cc:397: if (array->IsFixedArray()) {
maybe you should assert that array is actually a fixed array, cause no other lo
chunk should be have scan_on_scavenge().

[Erik Corry, 2011-03-28 15:56:07]

http://codereview.chromium.org/6745033/diff/19002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6745033/diff/19002/src/heap.cc#newcode492
src/heap.cc:492: IncrementalMarking::set_should_hurry(false);
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> I think nicer place to clean the flag is in the IncrementalMarking::Finalize()

Done and made it private.

http://codereview.chromium.org/6745033/diff/19002/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6745033/diff/19002/src/heap.h#newcode244
src/heap.h:244: // The fullness of the store buffer when we started to scan the
current page.
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> I can't grok the comment.

Rewritten.

http://codereview.chromium.org/6745033/diff/19002/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/6745033/diff/19002/src/mark-compact.cc#newcode...
src/mark-compact.cc:1729:
StoreBuffer::EnterDirectlyIntoStoreBuffer(reinterpret_cast<Address>(p));
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> This visitor is used to visit new space objects.
> 
> Their slots should not be entered into store-buffer.

Good catch!

http://codereview.chromium.org/6745033/diff/19002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6745033/diff/19002/src/objects-inl.h#newcode834
src/objects-inl.h:834: if (Heap::InNewSpace(value)) \
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> curly brackets around if's body.

Done.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode334
src/spaces.h:334: owner_ = reinterpret_cast<Address>(space) + kFailureTag;
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> add assertion that checks that space was properly aligned.

Done.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode449
src/spaces.h:449: // The identity of the owning space.  This is tagged as a heap
pointer, but
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> Update comment. Now this is tagged as a failure and failures can reside in
> objects.

Done.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode453
src/spaces.h:453: // This flag indicates that the page is not being tracked by
the store buffer.
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> Why are we using bool instead of built-in flags?

Because my next step will be to read it from generated code and for that the
code will be more compact if it is a bool rather than a bit we have to mask.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode459
src/spaces.h:459: int store_buffer_counter_;
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> You added 2 fields but kHeaderSize was increased only by 1 kPointerSize.

There is a static assert that checks that the size is sufficient. Apparently it
was too big before.

http://codereview.chromium.org/6745033/diff/19002/src/spaces.h#newcode2051
src/spaces.h:2051: // FALL THROUGH!
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> this comment does not look like something we usually write.

lowercaseified.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc
File src/store-buffer.cc (right):

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode171
src/store-buffer.cc:171: ExemptPopularPages(97, ((Page::kPageSize /
kPointerSize) / 97) / 8);
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> this looks like a repeatable pattern.
> loop with array of prime numbers?

Done.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode231
src/store-buffer.cc:231: containing_chunk =
MemoryChunk::FromAnyPointerAddress(addr);
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> This code can be pretty slow on certain patterns.

Yes.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode340
src/store-buffer.cc:340: void StoreBuffer::Verify() {
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> I think we should have a way to verify store-buffer or we might have some
> troubles debugging strange crashes.

I will consider this for another change.

http://codereview.chromium.org/6745033/diff/19002/src/store-buffer.cc#newcode397
src/store-buffer.cc:397: if (array->IsFixedArray()) {
On 2011/03/28 15:13:19, Vyacheslav Egorov wrote:
> maybe you should assert that array is actually a fixed array, cause no other
lo
> chunk should be have scan_on_scavenge().

Done.