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 (right): http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode974 src/heap.cc:974: StoreBuffer::Verify(); we should move verification of storebuffer into Heap::Verify(). add TODO(gc)? http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode1006 src/heap.cc:1006: StoreBuffer::Clean(); Why we do this only in debug? Comment? http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4201 src/heap.cc:4201: uint32_t Heap::IterateDirtyRegions( 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?) http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4222 src/heap.cc:4222: space->free_list()->Zap(); Why do we need to Zap free_list? Can we zap it with a special value to detect cases when we have slots of dead objects in store buffer? http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4224 src/heap.cc:4224: StoreBuffer::SortUniq(); 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. http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4247 src/heap.cc:4247: if (o->IsSmi()) continue; InNewSpace(Object* o) can't return true for a smi. http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4272 src/heap.cc:4272: space->free_list()->Zap(); Can we zap it with a special value to detect cases when we have slots of dead objects in store buffer? http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4290 src/heap.cc:4290: Address map_aligned_current = MapStartAlign(page->ObjectAreaStart()); both ObjectAreaStart and watermark should be aligned by MapSize. this is no-ops. consider removing them http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4307 src/heap.cc:4307: for ( ; current < limit; current++) { This loop looks familiar. I saw similar one above. http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4309 src/heap.cc:4309: if (o->IsSmi()) continue; InNewSpace(Object* o) can't return true for a smi. http://codereview.chromium.org/6309012/diff/15001/src/heap.cc#newcode4335 src/heap.cc:4335: for ( ; current < limit; current++) { This loop looks familiar. Consider moving it to separate function. 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)) || \ 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. 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); Kill all constants/fields related to cardmarking. Not just their usages. No mercy! 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(); This way you zap the next field of the cur_node! It seems reasonable to teach FreeListNode Zap itself. http://codereview.chromium.org/6309012/diff/15001/src/spaces.cc#newcode1611 src/spaces.cc:1611: FreeListNode* cur_node = FreeListNode::FromAddress(cur_addr); cur_node->Zap(); see above. 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)); this look really strange. should not you check for sizeof(int)? does it worth it to have different impls of CompareAddresses? 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. This comment is a bit confusing. It removes all duplicates only if called on a sorted buffer. 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))) { There is (now) Heap::InNewSpace(Address addr) http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode142 src/store-buffer.cc:142: ZapHashTables(); 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... 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. If we do not have a way to recover from this condition should we put UNREACHABLE() or at least TODO here? http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode210 src/store-buffer.cc:210: Heap::OldPointerSpaceCheckStoreBuffer(Heap::WATERMARK_SHOULD_BE_VALID); I don't see a reason why this methods should be in the Heap. http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.cc#newcode282 src/store-buffer.cc:282: printf("store buffer overfull\n"); kill printf 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(); Should not be this wrapped into #ifdef DEBUG? Also Clean is a very confusing name. http://codereview.chromium.org/6309012/diff/15001/src/store-buffer.h#newcode71 src/store-buffer.h:71: static void SortUniq(); Better name? http://codereview.chromium.org/6309012/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
