I like this idea. It is fairly simple and the shrinking of objects without actually touching them is cool. :)
http://codereview.chromium.org/3329019/diff/1/2 File src/arm/builtins-arm.cc (right): http://codereview.chromium.org/3329019/diff/1/2#newcode534 src/arm/builtins-arm.cc:534: // Use r7 for holding undefined which is used in several places below. No reason to load undefined into r7 here anymore I think. I think we are only using r7 in one place now. So just load filler into r7 just before loop and undefined into r7 just before the other loop. That way you don't have to save and restore r7 when finalizing the instance size. http://codereview.chromium.org/3329019/diff/1/2#newcode573 src/arm/builtins-arm.cc:573: // rcx: shared function info rcx -> r3 Maybe just remove the comment since you just loaded it. Seems obvious. http://codereview.chromium.org/3329019/diff/1/2#newcode578 src/arm/builtins-arm.cc:578: __ strb(r4, constructor_count); Doesn't this means that the count will wrap around and you will redo the map tree traversal again later (at which point you cannot gain anything). It would be nice to avoid that. Edit: It doesn't mean that it is done again - I see that you change the stub when calling finalize - could you add a comment about that here? http://codereview.chromium.org/3329019/diff/1/2#newcode581 src/arm/builtins-arm.cc:581: __ push(r1); Using Push and Pop macros you should be able to use strm and ldrm for r2 and r1 here. http://codereview.chromium.org/3329019/diff/1/2#newcode618 src/arm/builtins-arm.cc:618: // Fill all the in-object properties with undefined. with undefined -> with one-word fillers. http://codereview.chromium.org/3329019/diff/1/5 File src/heap.cc (right): http://codereview.chromium.org/3329019/diff/1/5#newcode2698 src/heap.cc:2698: Object* filler; Do we need the conditional here? Is this because of internal fields from API functions? If that is the case it requires a comment! :) http://codereview.chromium.org/3329019/diff/1/7 File src/ia32/assembler-ia32.h (right): http://codereview.chromium.org/3329019/diff/1/7#newcode598 src/ia32/assembler-ia32.h:598: void dec_b(const Operand& dst); Do you need to update the disassembler as well or does it already handle this? http://codereview.chromium.org/3329019/diff/1/9 File src/objects-inl.h (right): http://codereview.chromium.org/3329019/diff/1/9#newcode1346 src/objects-inl.h:1346: void JSObject::InitializeBody(int object_size, Object* value) { Please add an assert that the value is not in new space. There is no write barrier in this code. http://codereview.chromium.org/3329019/diff/1/10 File src/objects.cc (right): http://codereview.chromium.org/3329019/diff/1/10#newcode3298 src/objects.cc:3298: if (descriptors->GetType(i) == MAP_TRANSITION || Don't we have an IsTransition method? We should have so this will not break if we add a new transition type. http://codereview.chromium.org/3329019/diff/1/11 File src/objects.h (right): http://codereview.chromium.org/3329019/diff/1/11#newcode3457 src/objects.h:3457: // Inobject slack tracking: I wonder if we really have to keep track of this as we build the map transitions? Seems a little heavy to have all these counters going and having to update them when we introduce a new map transition. Is the traversal of the map tree fast enough that we could do one pass to find the new number and one pass to set it (bailing out during the iteration if we see too many maps)? Maybe it is too expensive, but it seems much simpler to me. http://codereview.chromium.org/3329019/diff/1/11#newcode3493 src/objects.h:3493: inline void InitializeInobjectSlackTracking(); Please add comments explaining each of these. http://codereview.chromium.org/3329019/diff/1/11#newcode3721 src/objects.h:3721: static const int kSize = kInobjectSlackTrackingOffset + 4; Isn't kIntSize the right thing here? http://codereview.chromium.org/3329019/diff/1/12 File src/runtime.cc (right): http://codereview.chromium.org/3329019/diff/1/12#newcode6781 src/runtime.cc:6781: // If the constructor isn't a proper function we throw a type error. This seems a bit strange to me. When does this happen and why don't you just return undefined in that case? Do you ever use the result of this call? http://codereview.chromium.org/3329019/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
