first round of comments
http://codereview.chromium.org/6639024/diff/1/src/debug.cc File src/debug.cc (right): http://codereview.chromium.org/6639024/diff/1/src/debug.cc#newcode1877 src/debug.cc:1877: // scripts which are no longer referenced. Comment is outdated add more about precise sweeping? http://codereview.chromium.org/6639024/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4217 src/heap.cc:4217: typedef bool (*CheckStoreBufferFilter)(Object**addr); space after ** http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4224 src/heap.cc:4224: mod < Map:: kPointerFieldsEndOffset; remove space after :: http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4372 src/heap.cc:4372: // This function iterates over all the pointers in a paged space in the heap, Special garbage section is utterly confusing. Maybe draw page layout? [ header | garbage section | whatever ] http://codereview.chromium.org/6639024/diff/1/src/heap.h File src/heap.h (right): http://codereview.chromium.org/6639024/diff/1/src/heap.h#newcode768 src/heap.h:768: static const int kSweepPreciselyMask = 2; maybe better hide the concept of conservative and precise sweep and just call it kEnsureIterableMask or something like that? http://codereview.chromium.org/6639024/diff/1/src/mark-compact.cc File src/mark-compact.cc (right): http://codereview.chromium.org/6639024/diff/1/src/mark-compact.cc#newcode1936 src/mark-compact.cc:1936: char kStartTable[kStartTableSize * kStartTableStride] = { Stride and Size are really mysterious names --- more comments would be great. http://codereview.chromium.org/6639024/diff/1/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/6639024/diff/1/src/objects-inl.h#newcode339 src/objects-inl.h:339: return instance_type == BYTE_ARRAY_TYPE || instance_type == FILLER_TYPE; maybe it is really time to make filler_array_type or whatever. http://codereview.chromium.org/6639024/diff/1/src/objects.h File src/objects.h (right): http://codereview.chromium.org/6639024/diff/1/src/objects.h#newcode2597 src/objects.h:2597: inline int Size() { return RoundUp(length() + kHeaderSize, kPointerSize); } What's the difference from ByteArraySize()? http://codereview.chromium.org/6639024/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): http://codereview.chromium.org/6639024/diff/1/src/profile-generator.cc#newcode2374 src/profile-generator.cc:2374: Heap::EnsureHeapIsIterable(); Sometimes we call Ensure, sometimes we don't. This is too implicit. Can we move this decision into iterator constructor? http://codereview.chromium.org/6639024/diff/1/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/6639024/diff/1/src/runtime.cc#newcode10527 src/runtime.cc:10527: Heap::EnsureHeapIsIterable(); This is very fragile. We definitely should encapsulate this decision in the iterator itself. http://codereview.chromium.org/6639024/diff/1/src/spaces.h File src/spaces.h (right): http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode358 src/spaces.h:358: static const int kObjectStartAlignment = 32 * kPointerSize; Why did not you just modify body offset? http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode430 src/spaces.h:430: private: does not make much sense to me to have a private section for a friend declaration. http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode843 src/spaces.h:843: // objects. what if we allocate inside page in the hole? how starting from the top will help? or top is not top but something in the middle? http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode858 src/spaces.h:858: inline HeapObject* Next() { What if bytearray is really something important why do we skip it? We are using bytearrays to keep relocation data for example. http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode1046 src/spaces.h:1046: static const uintptr_t kFreeListZapValue = 0xfeed1eaf; please move it to other zap values. http://codereview.chromium.org/6639024/diff/1/test/mjsunit/debug-script.js File test/mjsunit/debug-script.js (right): http://codereview.chromium.org/6639024/diff/1/test/mjsunit/debug-script.js#newcode44 test/mjsunit/debug-script.js:44: print(scripts[i].name); please remove http://codereview.chromium.org/6639024/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
