Issue 6309012: * Complete new store buffer on ia32. The store buffer now covers...

Issue 6309012: * Complete new store buffer on ia32. The store buffer now covers... (Closed)

Created:
9 years, 11 months ago by Erik Corry

Modified:
9 years, 7 months ago

Reviewers:
Vyacheslav Egorov (Chromium)

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/experimental/gc/

Visibility:
Public.

Description

* Complete new store buffer on ia32. The store buffer now covers all the places it needs and can be verified against a scan of old space. Committed: http://code.google.com/p/v8/source/detail?r=6443

Patch Set 2 : '' #

Total comments: 50

Created: 9 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+432 lines, -145 lines)			Patch
M	src/factory.cc	View	1 chunk	+7 lines, -1 line	2 comments	Download
M	src/heap.h	View	2 chunks	+9 lines, -7 lines	0 comments	Download
M	src/heap.cc	View	6 chunks	+155 lines, -77 lines	22 comments	Download
M	src/heap-inl.h	View	2 chunks	+4 lines, -28 lines	0 comments	Download
M	src/ia32/ic-ia32.cc	View	3 chunks	+0 lines, -4 lines	0 comments	Download
M	src/mark-compact.cc	View	1 chunk	+1 line, -1 line	0 comments	Download
M	src/objects-inl.h	View	2 chunks	+1 line, -7 lines	2 comments	Download
M	src/serialize.h	View	1 chunk	+2 lines, -1 line	0 comments	Download
M	src/serialize.cc	View	2 chunks	+8 lines, -1 line	0 comments	Download
M	src/spaces.h	View	4 chunks	+12 lines, -0 lines	0 comments	Download
M	src/spaces.cc	View	2 chunks	+35 lines, -5 lines	6 comments	Download
M	src/store-buffer.h	View	1 chunk	+33 lines, -0 lines	4 comments	Download
M	src/store-buffer.cc	View	5 chunks	+165 lines, -13 lines	14 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Chromium)

LGTM http://codereview.chromium.org/6309012/diff/15001/src/factory.cc File src/factory.cc (right): http://codereview.chromium.org/6309012/diff/15001/src/factory.cc#newcode765 src/factory.cc:765: Handle<SharedFunctionInfo> a = NooShFuIh(name); revert http://codereview.chromium.org/6309012/diff/15001/src/heap.cc File src/heap.cc ...

9 years, 11 months ago (2011-01-21 18:18:18 UTC) #2

Erik Corry

9 years, 11 months ago (2011-01-24 13:55:59 UTC) #3

http://codereview.chromium.org/6309012/diff/15001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6309012/diff/15001/src/factory.cc#newcode765
src/factory.cc:765: Handle<SharedFunctionInfo> a = NooShFuIh(name);
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> revert

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode974
src/heap.cc:974: StoreBuffer::Verify();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> we should move verification of storebuffer into Heap::Verify().
> 
> add TODO(gc)?

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode1006
src/heap.cc:1006: StoreBuffer::Clean();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Why we do this only in debug?

When we actually use the store buffer info it will get cleaned and rebuilt as a
side effect of using it.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4201
src/heap.cc:4201: uint32_t Heap::IterateDirtyRegions(
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> If you decided to kill cardmarking wb maybe you will do it completely without
> leaving any trace (search for all ENABLE_CARDMARKING_ ifdefs and kill them?)

That is certainly the plan.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4222
src/heap.cc:4222: space->free_list()->Zap();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Why do we need to Zap free_list?

We zap the free list so that we can walk the whole old space (ignoring object
boundaries) and verify that there are no new space pointers that are missing in
the store buffer.

The issue you raise is the opposite one, the issue of entries in the store
buffer that are inside dead objects.  I think a magic zap value can take care of
both issues.
> 
> Can we zap it with a special value to detect cases when we have slots of dead
> objects in store buffer?

Yes.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4224
src/heap.cc:4224: StoreBuffer::SortUniq();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Verifying StoreBuffer has an unfortunate side-effect of uniqifying it.
> 
> Are we going to uniqify it on each collection? If not --- storebuffer with
> verification and without will behave differently.

I think we will be sorting and uniqifying on each collection.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4247
src/heap.cc:4247: if (o->IsSmi()) continue;
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> InNewSpace(Object* o) can't return true for a smi.

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4272
src/heap.cc:4272: space->free_list()->Zap();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Can we zap it with a special value to detect cases when we have slots of dead
> objects in store buffer?

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4290
src/heap.cc:4290: Address map_aligned_current =
MapStartAlign(page->ObjectAreaStart());
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> both ObjectAreaStart and watermark should be aligned by MapSize.
> 
> this is no-ops. consider removing them

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4307
src/heap.cc:4307: for ( ; current < limit; current++) {
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> This loop looks familiar.
> 
> I saw similar one above.

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4309
src/heap.cc:4309: if (o->IsSmi()) continue;
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> InNewSpace(Object* o) can't return true for a smi.

Done.

http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4335
src/heap.cc:4335: for ( ; current < limit; current++) {
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> This loop looks familiar.
> 
> Consider moving it to separate function.

Done.

http://codereview.chromium.org/6309012/diff/15001/src/objects-inl.h
File src/objects-inl.h (left):

http://codereview.chromium.org/6309012/diff/15001/src/objects-inl.h#oldcode807
src/objects-inl.h:807: !Heap::InNewSpace(READ_FIELD(object, offset)) || \
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> This check was here for some reason.
> 
> Somewhere in built-ins we were omitting the WB because we know that WB already
> remembers this slot.

Done.

http://codereview.chromium.org/6309012/diff/15001/src/spaces.cc
File src/spaces.cc (left):

http://codereview.chromium.org/6309012/diff/15001/src/spaces.cc#oldcode2315
src/spaces.cc:2315: ASSERT(marks == Page::kAllRegionsDirtyMarks);
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Kill all constants/fields related to cardmarking. Not just their usages.
> 
> No mercy!

Soon.

Nice time stamp.

http://codereview.chromium.org/6309012/diff/15001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/6309012/diff/15001/src/spaces.cc#newcode1594
src/spaces.cc:1594: Address payload_start = ba->GetDataStartAddress();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> This way you zap the next field of the cur_node!
> 
> It seems reasonable to teach FreeListNode Zap itself.

Good catch.  Done.

http://codereview.chromium.org/6309012/diff/15001/src/spaces.cc#newcode1611
src/spaces.cc:1611: FreeListNode* cur_node =
FreeListNode::FromAddress(cur_addr);
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> cur_node->Zap();
> 
> see above.

Done.

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

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode114
src/store-buffer.cc:114: ASSERT(sizeof(1) == sizeof(a));
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> this look really strange. should not you check for sizeof(int)?

The linter objects to sizeof on types.

> 
> does it worth it to have different impls of CompareAddresses?

Not sure.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode123
src/store-buffer.cc:123: // Remove duplicates and cells that do not point at new
space.
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> This comment is a bit confusing.
> 
> It removes all duplicates only if called on a sorted buffer.

Comment fixed.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode129
src/store-buffer.cc:129: if
(Heap::new_space()->Contains(*reinterpret_cast<Address*>(current))) {
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> There is (now) Heap::InNewSpace(Address addr)

Done.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode142
src/store-buffer.cc:142: ZapHashTables();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Instead of zaping hashtables completely we can update them instead (zap only
> positions corresponding to non-newspace pointers). But it might not be worth
> it...

I think it isn't worth it.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode156
src/store-buffer.cc:156: // We don't currently have a way to recover from this
condition.
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> If we do not have a way to recover from this condition should we put
> UNREACHABLE() or at least TODO here?

I will update the comment.  We can recover in terms of correctness, not
performance.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode210
src/store-buffer.cc:210:
Heap::OldPointerSpaceCheckStoreBuffer(Heap::WATERMARK_SHOULD_BE_VALID);
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> I don't see a reason why this methods should be in the Heap.

They use the constants like Heap::WATERMARK_SHOULD_BE_VALID and it turns out to
be difficult to move them out of heap.h and heap.cc.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode282
src/store-buffer.cc:282: printf("store buffer overfull\n");
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> kill printf

Done.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.h
File src/store-buffer.h (right):

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.h#newcode70
src/store-buffer.h:70: static void Clean();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Should not be this wrapped into #ifdef DEBUG?
> 
> Also Clean is a very confusing name.

Done.

http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.h#newcode71
src/store-buffer.h:71: static void SortUniq();
On 2011/01/21 18:18:18, Vyacheslav Egorov wrote:
> Better name?

I like it.

Expand Messages | Collapse Messages