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#newcode1729 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(). http://codereview.chromium.org/6745033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
