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