Addressed comments, PTAL.
https://chromiumcodereview.appspot.com/10575032/diff/1/src/ic.cc File src/ic.cc (left): https://chromiumcodereview.appspot.com/10575032/diff/1/src/ic.cc#oldcode1443 src/ic.cc:1443: ASSERT(lookup->type() != NULL_DESCRIPTOR); As discussed offline, replaced with IsFound(). On 2012/06/25 11:21:57, Michael Starzinger wrote:
We should actually keep this and assert against NONEXISTENT now.
https://chromiumcodereview.appspot.com/10575032/diff/1/src/ic.cc#oldcode1945 src/ic.cc:1945: ASSERT(lookup->type() != NULL_DESCRIPTOR); On 2012/06/25 11:21:57, Michael Starzinger wrote:
Likewise.
Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects-inl.h File src/objects-inl.h (right): https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects-inl.h#newcode3487 src/objects-inl.h:3487: void Map::SetOwnBitField3(int value) { On 2012/06/25 11:21:57, Michael Starzinger wrote:
That method name is mighty confusing! Is should actually be named ClearDescriptorArray I guess.
Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7346 src/objects.cc:7346: static void RightTrimFixedArray(Heap* heap, FixedArray* elms, int to_trim) { On 2012/06/25 11:21:57, Michael Starzinger wrote:
Add a comment that this function is only called from within the GC.
Otherwise
the live-bytes counter will be inconsistent.
Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7355 src/objects.cc:7355: STATIC_ASSERT(FixedArray::kHeaderSize == 2 * kPointerSize); On 2012/06/25 11:21:57, Michael Starzinger wrote:
These asserts are not needed for left-trimming!
Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7357 src/objects.cc:7357: const int len = elms->length(); On 2012/06/25 11:21:57, Michael Starzinger wrote:
Add an ASSERT(to_trim < len), other cases aren't handled correctly
here. Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7359 src/objects.cc:7359: Address new_end = elms->address() + FixedArray::kHeaderSize + On 2012/06/25 11:21:57, Michael Starzinger wrote:
Better use "elms->address() + FixedArray::SizeFor(len - to_trim)"
here. Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7361 src/objects.cc:7361: if (to_trim > FixedArray::kHeaderSize / kPointerSize && On 2012/06/25 11:21:57, Michael Starzinger wrote:
This condition doesn't make much sense to me. And as discussed
offline, the
zapping is probably only needed for debug mode with the new GC.
Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7364 src/objects.cc:7364: // formerly part of the array so that the GC (aided by the card-based On 2012/06/25 11:21:57, Michael Starzinger wrote:
Comment no longer applies.
Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7389 src/objects.cc:7389: static bool ClearDescriptor(Heap* heap, On 2012/06/25 11:21:57, Michael Starzinger wrote:
We should give this function the "ClearNonLive" prefix as well. How
about
"ClearNonLiveTransitionsFromDescriptor"?
Also a comment describing the semantics of the return value would
nice. Done. https://chromiumcodereview.appspot.com/10575032/diff/1/src/objects.cc#newcode7454 src/objects.cc:7454: while (descriptor_index < d->number_of_descriptors() && On 2012/06/25 11:21:57, Michael Starzinger wrote:
Is there a strong reason why this loop is split into two parts and
interleaved
with the clearing of elements transitions? Is seems overly
complicated. Can we
merge these two loops into one?
Done. https://chromiumcodereview.appspot.com/10575032/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
