Dynamically determine optimal instance size....

Dynamically determine optimal instance size.

The number of inobject properties used to be derived from the number
of this property assignments in the constructor (and increased by 2 to
allow for properties added later). This very often leads to wasted inobject 
slots.

This patch reclaims some of the unused inobject space by the following method:
 - for each constructor function the first several objects are allocated using the initial 
   ("generous) instance size estimation (this is called 'tracking phase').
 - during the tracking phase map transitions are tracked and actual property counts are collected.
 - at the end of the tracking phase instance sizes in the maps are decreased if necessary
   (starting with the function's initial map and traversing the transition tree).
 - all further allocation use more realistic instance size estimation.

Shrinking generously allocated objects without costly heap traversal is made possible 
by initializing their inobject properties with one_pointer_filler_map (instead of undefined). 

The initial slack for the generous allocation is increased from 2 to 6 which really helps some tests.


Committed: http://code.google.com/p/v8/source/detail?r=5510

[Vladislav Kaznacheev, 2010-09-10 09:29:42]

This is almost ready for commit with two exceptions: 
- I am going to add a test for multiple contexts 
- I want to make sure I do not break anything related to API functions

[Mads Ager (chromium), 2010-09-10 12:07:13]

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?

[antonm, 2010-09-10 15:28:50]

Pretty neat

http://codereview.chromium.org/3329019/diff/1/8
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/3329019/diff/1/8#newcode191
src/ia32/builtins-ia32.cc:191: // To allow for truncation
nit: missing dot at the end of the comment.

http://codereview.chromium.org/3329019/diff/1/8#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx, Factory::one_pointer_filler_map());
if we allocate API objects with the code, it might be a problem due to internal
fields usage (see also Mads' comment in C++ part).  Overall this difference in
the logic might be dangerous

http://codereview.chromium.org/3329019/diff/1/10
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/1/10#newcode3294
src/objects.cc:3294: map->set_visitor_id(StaticVisitorBase::GetVisitorId(map));
why is this needed?  could you add a comment?

http://codereview.chromium.org/3329019/diff/1/10#newcode5417
src/objects.cc:5417: if (unused_inobject_properties != 0) {
if unused_inobject_properties == 0, how would this code behave?  apparently
you'll go into started, but not in progress state, is it intended?

http://codereview.chromium.org/3329019/diff/1/10#newcode5447
src/objects.cc:5447:
set_construct_stub(Builtins::builtin(Builtins::JSConstructStubGeneric));
do we need an assert that current constuct_stub is JSConstructStubCountdown?

http://codereview.chromium.org/3329019/diff/1/10#newcode5454
src/objects.cc:5454: // Make the counter runs out the next time.
nit: runs -> run

http://codereview.chromium.org/3329019/diff/1/11
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/1/11#newcode3239
src/objects.h:3239: // via map transitions. Should be applied only to a map at
the top of the
nit: top -> root (of the transition tree)

http://codereview.chromium.org/3329019/diff/1/11#newcode3248
src/objects.h:3248: static const int kMaxShrinkableTreeSize = 50;
just curious: do we have any data on tree size distribution?

http://codereview.chromium.org/3329019/diff/1/11#newcode3713
src/objects.h:3713: #warning Unknown byte ordering
#error?

http://codereview.chromium.org/3329019/diff/1/12
File src/runtime.cc (right):

http://codereview.chromium.org/3329019/diff/1/12#newcode6763
src/runtime.cc:6763: if (first_allocation &&
!shared->IsInobjectSlackTrackingInProgress()) {
isn't it true that IsInobjectSlackTrackingInProgress => Started, hence you don't
need && at all?

[Vitaly Repeshko, 2010-09-14 13:24:00]

A few more comments.

http://codereview.chromium.org/3329019/diff/1/10
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/1/10#newcode5434
src/objects.cc:5434: set_inobject_slack_estimation(unused_inobject_properties);
If unused_inobject_properties is 0, then this half-cancels the tracking
(IsInobjectSlackTrackingInProgress() becomes false). Is this a problem? If not,
it's still too subtle and requires a comment.

http://codereview.chromium.org/3329019/diff/1/10#newcode5454
src/objects.cc:5454: // Make the counter runs out the next time.
Why do we have to postpone it? Can we call Complete...Tracking() here?

http://codereview.chromium.org/3329019/diff/1/11
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/1/11#newcode3246
src/objects.h:3246: // The size limit for a map transition tree that can be
safely processed
If we have a "class" with 51 functions, does this limit mean we'll always be too
generous when allocating the instances? If yes, this could be bad for memory
usage. I understand that this limit should not affect the normal usage of
putting the functions on the prototype, but some OO frameworks put the functions
on the instance.

Please have a look at http://codereview.chromium.org/3385002 where I attempt to
do a stackless traversal of the map tree.

http://codereview.chromium.org/3329019/diff/1/11#newcode3248
src/objects.h:3248: static const int kMaxShrinkableTreeSize = 50;
kMaxShrinkable*Transition*TreeSize?

http://codereview.chromium.org/3329019/diff/1/11#newcode3457
src/objects.h:3457: // Inobject slack tracking:
This comment needs to also explain how different contexts sharing a function are
affected by tracking.

http://codereview.chromium.org/3329019/diff/1/11#newcode3463
src/objects.h:3463: // To reclaims unused inobject space we:
reclaims -> reclaim.

http://codereview.chromium.org/3329019/diff/1/11#newcode3488
src/objects.h:3488: inline bool IsInobjectSlackTrackingInProgress();
Please describe the full state machine here.

[Vladislav Kaznacheev, 2010-09-20 17:26:59]

I have solved the problem with GC hitting while the inobject slack tracking is
in progress.

Comments addressed.

Still need to add a test for multiple contexts.

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.
On 2010/09/10 12:07:13, Mads Ager wrote:
> 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.

Done.

http://codereview.chromium.org/3329019/diff/1/2#newcode573
src/arm/builtins-arm.cc:573: // rcx: shared function info
On 2010/09/10 12:07:13, Mads Ager wrote:
> rcx -> r3
> 
> Maybe just remove the comment since you just loaded it. Seems obvious.

Done.

http://codereview.chromium.org/3329019/diff/1/2#newcode578
src/arm/builtins-arm.cc:578: __ strb(r4, constructor_count);
On 2010/09/10 12:07:13, Mads Ager wrote:
> 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?

Done.

http://codereview.chromium.org/3329019/diff/1/2#newcode581
src/arm/builtins-arm.cc:581: __ push(r1);
There is no Pop macro as far as I was able to see.
On 2010/09/10 12:07:13, Mads Ager wrote:
> 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.
On 2010/09/10 12:07:13, Mads Ager wrote:
> with undefined -> with one-word fillers.

Done.

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;
Added a comment.

On 2010/09/10 12:07:13, Mads Ager wrote:
> 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);
The disasssembler does not handle 'dec' properly, let me fix that in a separate
patch.

On 2010/09/10 12:07:13, Mads Ager wrote:
> Do you need to update the disassembler as well or does it already handle this?

http://codereview.chromium.org/3329019/diff/1/8
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/3329019/diff/1/8#newcode191
src/ia32/builtins-ia32.cc:191: // To allow for truncation
On 2010/09/10 15:28:50, antonm wrote:
> nit: missing dot at the end of the comment.

Done.

http://codereview.chromium.org/3329019/diff/1/8#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx, Factory::one_pointer_filler_map());
We never allocate API objects with this code. I have added an assert to the
corresponding C++ code in InitializeJSObjectFromMap (which will be called the
first time the constructor is called).
On 2010/09/10 15:28:50, antonm wrote:
> if we allocate API objects with the code, it might be a problem due to
internal
> fields usage (see also Mads' comment in C++ part).  Overall this difference in
> the logic might be dangerous

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) {
On 2010/09/10 12:07:13, Mads Ager wrote:
> Please add an assert that the value is not in new space. There is no write
> barrier in this code.

Done.

http://codereview.chromium.org/3329019/diff/1/10
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/1/10#newcode3294
src/objects.cc:3294: map->set_visitor_id(StaticVisitorBase::GetVisitorId(map));
This is needed because visitor id might depend on the instance size which we
just have changed.
Added the comment (the function moved).

On 2010/09/10 15:28:50, antonm wrote:
> why is this needed?  could you add a comment?

http://codereview.chromium.org/3329019/diff/1/10#newcode3298
src/objects.cc:3298: if (descriptors->GetType(i) == MAP_TRANSITION ||
On 2010/09/10 12:07:13, Mads Ager wrote:
> Don't we have an IsTransition method? We should have so this will not break if
> we add a new transition type.

Removed (doing stackless traversal now).

http://codereview.chromium.org/3329019/diff/1/10#newcode5417
src/objects.cc:5417: if (unused_inobject_properties != 0) {
It is intended. The IsInobjectSlackTrackingStarted method is now called
differently (more clearly).

On 2010/09/10 15:28:50, antonm wrote:
> if unused_inobject_properties == 0, how would this code behave?  apparently
> you'll go into started, but not in progress state, is it intended?

http://codereview.chromium.org/3329019/diff/1/10#newcode5434
src/objects.cc:5434: set_inobject_slack_estimation(unused_inobject_properties);
Removed the method
On 2010/09/14 13:24:01, Vitaly wrote:
> If unused_inobject_properties is 0, then this half-cancels the tracking
> (IsInobjectSlackTrackingInProgress() becomes false). Is this a problem? If
not,
> it's still too subtle and requires a comment.

http://codereview.chromium.org/3329019/diff/1/10#newcode5447
src/objects.cc:5447:
set_construct_stub(Builtins::builtin(Builtins::JSConstructStubGeneric));
On 2010/09/10 15:28:50, antonm wrote:
> do we need an assert that current constuct_stub is JSConstructStubCountdown?

Done.

http://codereview.chromium.org/3329019/diff/1/10#newcode5454
src/objects.cc:5454: // Make the counter runs out the next time.
Removed the method.

On 2010/09/14 13:24:01, Vitaly wrote:
> Why do we have to postpone it? Can we call Complete...Tracking() here?

http://codereview.chromium.org/3329019/diff/1/10#newcode5454
src/objects.cc:5454: // Make the counter runs out the next time.
Removed the method
On 2010/09/10 15:28:50, antonm wrote:
> nit: runs -> run

http://codereview.chromium.org/3329019/diff/1/11
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/1/11#newcode3239
src/objects.h:3239: // via map transitions. Should be applied only to a map at
the top of the
removed the method.
On 2010/09/10 15:28:50, antonm wrote:
> nit: top -> root (of the transition tree)

http://codereview.chromium.org/3329019/diff/1/11#newcode3246
src/objects.h:3246: // The size limit for a map transition tree that can be
safely processed
Many thanks for the code. Implemented stackless traversal, have no more fear.

On 2010/09/14 13:24:01, Vitaly wrote:
> If we have a "class" with 51 functions, does this limit mean we'll always be
too
> generous when allocating the instances? If yes, this could be bad for memory
> usage. I understand that this limit should not affect the normal usage of
> putting the functions on the prototype, but some OO frameworks put the
functions
> on the instance.
> 
> Please have a look at http://codereview.chromium.org/3385002 where I attempt
to
> do a stackless traversal of the map tree.

http://codereview.chromium.org/3329019/diff/1/11#newcode3248
src/objects.h:3248: static const int kMaxShrinkableTreeSize = 50;
doing stackless traversal now
On 2010/09/10 15:28:50, antonm wrote:
> just curious: do we have any data on tree size distribution?

http://codereview.chromium.org/3329019/diff/1/11#newcode3248
src/objects.h:3248: static const int kMaxShrinkableTreeSize = 50;
doing stackless traversal now.
On 2010/09/14 13:24:01, Vitaly wrote:
> kMaxShrinkable*Transition*TreeSize?

http://codereview.chromium.org/3329019/diff/1/11#newcode3457
src/objects.h:3457: // Inobject slack tracking:
Will experiment.
On 2010/09/10 12:07:13, Mads Ager wrote:
> 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#newcode3457
src/objects.h:3457: // Inobject slack tracking:
Removed on-the-fly transition tracking in favour of traversing the transition
tree at the tracking phase completion.
On 2010/09/10 12:07:13, Mads Ager wrote:
> 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#newcode3463
src/objects.h:3463: // To reclaims unused inobject space we:
On 2010/09/14 13:24:01, Vitaly wrote:
> reclaims -> reclaim.

Done.

http://codereview.chromium.org/3329019/diff/1/11#newcode3488
src/objects.h:3488: inline bool IsInobjectSlackTrackingInProgress();
On 2010/09/14 13:24:01, Vitaly wrote:
> Please describe the full state machine here.

Done. I think.

http://codereview.chromium.org/3329019/diff/1/11#newcode3493
src/objects.h:3493: inline void InitializeInobjectSlackTracking();
On 2010/09/10 12:07:13, Mads Ager wrote:
> Please add comments explaining each of these.

Done.

http://codereview.chromium.org/3329019/diff/1/11#newcode3713
src/objects.h:3713: #warning Unknown byte ordering
The only other place that checks this uses #warning. But you are right, this
must be an error.
On 2010/09/10 15:28:50, antonm wrote:
> #error?

http://codereview.chromium.org/3329019/diff/1/11#newcode3721
src/objects.h:3721: static const int kSize = kInobjectSlackTrackingOffset + 4;
On 2010/09/10 12:07:13, Mads Ager wrote:
> Isn't kIntSize the right thing here?

Removed.

http://codereview.chromium.org/3329019/diff/1/12
File src/runtime.cc (right):

http://codereview.chromium.org/3329019/diff/1/12#newcode6763
src/runtime.cc:6763: if (first_allocation &&
!shared->IsInobjectSlackTrackingInProgress()) {
NewJSObject call could change the state. Specifically, ...InProgress will be
false if Start... method skipped over the tracking phase. It is now a bit more
clear with new method names.
On 2010/09/10 15:28:50, antonm wrote:
> isn't it true that IsInobjectSlackTrackingInProgress => Started, hence you
don't
> need && at all?

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 gets called with non-function argument from natives fuzzer. Not sure what
is the right thing to do.
Returning undefined from the main branch.
On 2010/09/10 12:07:13, Mads Ager wrote:
> 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?

[antonm, 2010-09-21 11:48:55]

http://codereview.chromium.org/3329019/diff/15001/16001
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16001#newcode693
src/arm/builtins-arm.cc:693: }  // Otherwise r7 already holds undefined.
might be worth checking if FLAG_debug_code is enabled.

http://codereview.chromium.org/3329019/diff/15001/16006
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/3329019/diff/15001/16006#newcode598
src/ia32/assembler-ia32.h:598: void dec_b(const Operand& dst);
apparently still no changes to disassembler

http://codereview.chromium.org/3329019/diff/15001/16007
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16007#newcode191
src/ia32/builtins-ia32.cc:191: // To allow for truncation.
nit: do you want to unify with other platforms?  e.g. there is no such comment
in ARM port.

http://codereview.chromium.org/3329019/diff/15001/16007#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx, Factory::one_pointer_filler_map());
we should never come here for API functions, correct?  could we add Assert here?

http://codereview.chromium.org/3329019/diff/15001/16008
File src/objects-inl.h (right):

http://codereview.chromium.org/3329019/diff/15001/16008#newcode2575
src/objects-inl.h:2575: kInobjectSlackTrackingMapOffset)
it is somewhat weird to see initial_map accessor with
kInobjectSlackTrackingMapOffset.  Could the names be adjusted?

http://codereview.chromium.org/3329019/diff/15001/16009
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16009#newcode5446
src/objects.cc:5446: void SharedFunctionInfo::ReleaseInitialMap() {
apparently this CL is missing GC-part of the changes

http://codereview.chromium.org/3329019/diff/15001/16009#newcode5503
src/objects.cc:5503: void SharedFunctionInfo::CompleteInobjectSlackTracking() {
does that mean that after CompleteInobjectSlackTracking is over, another context
might initiate the process once again?

If yes, do we want it?

http://codereview.chromium.org/3329019/diff/15001/16010
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/15001/16010#newcode3243
src/objects.h:3243: typedef int (*TraverseCallback)(Map* map, void* data);
apparently you don't need return type int for the callback--it's never checked

http://codereview.chromium.org/3329019/diff/15001/16010#newcode3459
src/objects.h:3459: // after the constructor). There is no guarantee ther the
extra space
ther -> that?

http://codereview.chromium.org/3329019/diff/15001/16010#newcode3538
src/objects.h:3538: // May to go back from true to false if no constructed
objects survived GC.
May to go -> may go

Maybe rename to HasLiveObjects()?

http://codereview.chromium.org/3329019/diff/15001/16013
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16013#newcode1005
src/x64/builtins-x64.cc:1005: // To allow for truncation
nit: missing dot.

[Vladislav Kaznacheev, 2010-09-21 12:32:40]

Added missing mark-compact.cc.

http://codereview.chromium.org/3329019/diff/15001/16001
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16001#newcode693
src/arm/builtins-arm.cc:693: }  // Otherwise r7 already holds undefined.
On 2010/09/21 11:48:55, antonm wrote:
> might be worth checking if FLAG_debug_code is enabled.

Done.

http://codereview.chromium.org/3329019/diff/15001/16006
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/3329019/diff/15001/16006#newcode598
src/ia32/assembler-ia32.h:598: void dec_b(const Operand& dst);
Yes, I want to do that in a separate patch

On 2010/09/21 11:48:55, antonm wrote:
> apparently still no changes to disassembler

http://codereview.chromium.org/3329019/diff/15001/16007
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16007#newcode191
src/ia32/builtins-ia32.cc:191: // To allow for truncation.
On 2010/09/21 11:48:55, antonm wrote:
> nit: do you want to unify with other platforms?  e.g. there is no such comment
> in ARM port.

Done.

http://codereview.chromium.org/3329019/diff/15001/16007#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx, Factory::one_pointer_filler_map());
Added an assert at the top of the function on all 3 platforms.
On 2010/09/21 11:48:55, antonm wrote:
> we should never come here for API functions, correct?  could we add Assert
here?

http://codereview.chromium.org/3329019/diff/15001/16008
File src/objects-inl.h (right):

http://codereview.chromium.org/3329019/diff/15001/16008#newcode2575
src/objects-inl.h:2575: kInobjectSlackTrackingMapOffset)
Renamed to kInitialMapOffset
On 2010/09/21 11:48:55, antonm wrote:
> it is somewhat weird to see initial_map accessor with
> kInobjectSlackTrackingMapOffset.  Could the names be adjusted?

http://codereview.chromium.org/3329019/diff/15001/16009
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16009#newcode5446
src/objects.cc:5446: void SharedFunctionInfo::ReleaseInitialMap() {
Oops. Added.
On 2010/09/21 11:48:55, antonm wrote:
> apparently this CL is missing GC-part of the changes

http://codereview.chromium.org/3329019/diff/15001/16009#newcode5503
src/objects.cc:5503: void SharedFunctionInfo::CompleteInobjectSlackTracking() {
It does not mean that. StartInobjectSlackTracking uses a flag
(kHasConstructedObjects) to make sure the tracking is not started more than once
(unless we want it to in the case  when the tracking is cancelled by the GC).

On 2010/09/21 11:48:55, antonm wrote:
> does that mean that after CompleteInobjectSlackTracking is over, another
context
> might initiate the process once again?
> 
> If yes, do we want it?

http://codereview.chromium.org/3329019/diff/15001/16010
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/15001/16010#newcode3243
src/objects.h:3243: typedef int (*TraverseCallback)(Map* map, void* data);
On 2010/09/21 11:48:55, antonm wrote:
> apparently you don't need return type int for the callback--it's never checked

Done.

http://codereview.chromium.org/3329019/diff/15001/16010#newcode3459
src/objects.h:3459: // after the constructor). There is no guarantee ther the
extra space
On 2010/09/21 11:48:55, antonm wrote:
> ther -> that?

Done.

http://codereview.chromium.org/3329019/diff/15001/16010#newcode3538
src/objects.h:3538: // May to go back from true to false if no constructed
objects survived GC.
Fixed a typo. Renamed to LiveObjectsMayExist (because 'true' result is
conservative, not exact)
On 2010/09/21 11:48:55, antonm wrote:
> May to go -> may go
> 
> Maybe rename to HasLiveObjects()?

http://codereview.chromium.org/3329019/diff/15001/16013
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16013#newcode1005
src/x64/builtins-x64.cc:1005: // To allow for truncation
On 2010/09/21 11:48:55, antonm wrote:
> nit: missing dot.

Done.

[antonm, 2010-09-21 16:49:00]

LGTM, but I don't quite understand why you reserve disassembler for a separate
change, I'd prefer to see them go together.

http://codereview.chromium.org/3329019/diff/15001/16007
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16007#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx, Factory::one_pointer_filler_map());
On 2010/09/21 12:32:41, Vladislav Kaznacheev wrote:
> Added an assert at the top of the function on all 3 platforms.
> On 2010/09/21 11:48:55, antonm wrote:
> > we should never come here for API functions, correct?  could we add Assert
> here?
> 
> 

I meant an Assert compiled into the assembly code, but if it's too cumbersome,
that's fine.

http://codereview.chromium.org/3329019/diff/15001/16009
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16009#newcode5503
src/objects.cc:5503: void SharedFunctionInfo::CompleteInobjectSlackTracking() {
On 2010/09/21 12:32:41, Vladislav Kaznacheev wrote:
> It does not mean that. StartInobjectSlackTracking uses a flag
> (kHasConstructedObjects) to make sure the tracking is not started more than
once
> (unless we want it to in the case  when the tracking is cancelled by the GC).
> 

Yes, I am concerned that the very moment GC would clean this flag, the code
would run again.  I don't know if it's a problem or not.  My main concern is web
browser scenario, say, some navigation that makes the context go away and now
other contexts go into slower path again.

But it all speculations, feel free to leave it as is.

> On 2010/09/21 11:48:55, antonm wrote:
> > does that mean that after CompleteInobjectSlackTracking is over, another
> context
> > might initiate the process once again?
> > 
> > If yes, do we want it?
> 
>

http://codereview.chromium.org/3329019/diff/26002/28008
File src/mark-compact.cc (right):

http://codereview.chromium.org/3329019/diff/26002/28008#newcode1152
src/mark-compact.cc:1152: (map->bit_field2() & (1 <<
Map::kRelinkToSharedFunctionInfo))) {
maybe you should introduce functions like set_relink_to_shared_function_info to
Map class (cf. set_has_named_interceptor and alike)

[Vladislav Kaznacheev, 2010-09-22 11:33:06]

Addressed comments, waiting for Vitaly's LGTM.

http://codereview.chromium.org/3329019/diff/15001/16007
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16007#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx, Factory::one_pointer_filler_map());
Is there a difference? An Assert at the code generation phase is guaranteed to
prevent generating the code for the wrong set of arguments.

On 2010/09/21 16:49:00, antonm wrote:
> On 2010/09/21 12:32:41, Vladislav Kaznacheev wrote:
> > Added an assert at the top of the function on all 3 platforms.
> > On 2010/09/21 11:48:55, antonm wrote:
> > > we should never come here for API functions, correct?  could we add Assert
> > here?
> > 
> > 
> 
> I meant an Assert compiled into the assembly code, but if it's too cumbersome,
> that's fine.
>

http://codereview.chromium.org/3329019/diff/15001/16009
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/15001/16009#newcode5503
src/objects.cc:5503: void SharedFunctionInfo::CompleteInobjectSlackTracking() {
Clearing and restoring the flag is all done in GC, so to JS code can be executed
in between.

If the context that started the tracking goes away (e.g. with navigation) then
the next context that tries to create the object will restart tracking
_continuing the countdown from the previous value_. Regardless of how many times
this happens in 16 constructions the Complete... method will fire.

I have made an Assert at the start of the function stronger.
On 2010/09/21 16:49:00, antonm wrote:
> On 2010/09/21 12:32:41, Vladislav Kaznacheev wrote:
> > It does not mean that. StartInobjectSlackTracking uses a flag
> > (kHasConstructedObjects) to make sure the tracking is not started more than
> once
> > (unless we want it to in the case  when the tracking is cancelled by the
GC).
> > 
> 
> Yes, I am concerned that the very moment GC would clean this flag, the code
> would run again.  I don't know if it's a problem or not.  My main concern is
web
> browser scenario, say, some navigation that makes the context go away and now
> other contexts go into slower path again.
> 
> But it all speculations, feel free to leave it as is.
> 
> > On 2010/09/21 11:48:55, antonm wrote:
> > > does that mean that after CompleteInobjectSlackTracking is over, another
> > context
> > > might initiate the process once again?
> > > 
> > > If yes, do we want it?
> > 
> > 
> 
>

http://codereview.chromium.org/3329019/diff/26002/28008
File src/mark-compact.cc (right):

http://codereview.chromium.org/3329019/diff/26002/28008#newcode1152
src/mark-compact.cc:1152: (map->bit_field2() & (1 <<
Map::kRelinkToSharedFunctionInfo))) {
On 2010/09/21 16:49:00, antonm wrote:
> maybe you should introduce functions like set_relink_to_shared_function_info
to
> Map class (cf. set_has_named_interceptor and alike)

Done.

[Vitaly Repeshko, 2010-09-22 14:40:25]

LGTM with a few extra comments.

http://codereview.chromium.org/3329019/diff/40001/39007
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/3329019/diff/40001/39007#newcode598
src/ia32/assembler-ia32.h:598: void dec_b(const Operand& dst);
Does the disassembler support this already? If not, please extend it (and also
test-disasm-ia32).

http://codereview.chromium.org/3329019/diff/40001/39009
File src/mark-compact.cc (right):

http://codereview.chromium.org/3329019/diff/40001/39009#newcode1151
src/mark-compact.cc:1151: if (map->IsMarked() &&
map->attached_to_shared_function_info()) {
This needs a short comment.

http://codereview.chromium.org/3329019/diff/40001/39011
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/40001/39011#newcode5516
src/objects.cc:5516: set_expected_nof_properties(expected_nof_properties() -
slack);
Assert expected_nof_properties() - slack >= 0?

http://codereview.chromium.org/3329019/diff/40001/39012
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/40001/39012#newcode3499
src/objects.h:3499: // - The constructor is called from another context.
As we discussed this probably applies also to the case when a few closures from
a single context share a SharedFunctionInfo. If yes, please update the comment.

http://codereview.chromium.org/3329019/diff/40001/39013
File src/runtime.cc (right):

http://codereview.chromium.org/3329019/diff/40001/39013#newcode6282
src/runtime.cc:6282: static void SetInlineConstructStub(Handle<JSFunction>
function) {
This name is misleading. It makes the reader think an inline stub
unconditionally set. Consider adding "IfPossible" suffix or "Maybe" prefix.

http://codereview.chromium.org/3329019/diff/40001/39013#newcode6380
src/runtime.cc:6380: // If the constructor isn't a proper function we throw a
type error.
Why do have to throw this specific error? Why can't CONVERT_ARG_CHECKED be used
here? If there is a reason, please update the comment.

[Vladislav Kaznacheev, 2010-09-23 08:38:16]

Addressed the comments. Submitting.

Thanks for the comments, everyone!

http://codereview.chromium.org/3329019/diff/40001/39007
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/3329019/diff/40001/39007#newcode598
src/ia32/assembler-ia32.h:598: void dec_b(const Operand& dst);
Will add support in a separate patch.

On 2010/09/22 14:40:25, Vitaly wrote:
> Does the disassembler support this already? If not, please extend it (and also
> test-disasm-ia32).

http://codereview.chromium.org/3329019/diff/40001/39009
File src/mark-compact.cc (right):

http://codereview.chromium.org/3329019/diff/40001/39009#newcode1151
src/mark-compact.cc:1151: if (map->IsMarked() &&
map->attached_to_shared_function_info()) {
On 2010/09/22 14:40:25, Vitaly wrote:
> This needs a short comment.

Done.

http://codereview.chromium.org/3329019/diff/40001/39011
File src/objects.cc (right):

http://codereview.chromium.org/3329019/diff/40001/39011#newcode5516
src/objects.cc:5516: set_expected_nof_properties(expected_nof_properties() -
slack);
On 2010/09/22 14:40:25, Vitaly wrote:
> Assert expected_nof_properties() - slack >= 0?

Done.

http://codereview.chromium.org/3329019/diff/40001/39012
File src/objects.h (right):

http://codereview.chromium.org/3329019/diff/40001/39012#newcode3499
src/objects.h:3499: // - The constructor is called from another context.
On 2010/09/22 14:40:25, Vitaly wrote:
> As we discussed this probably applies also to the case when a few closures
from
> a single context share a SharedFunctionInfo. If yes, please update the
comment.

Done.

http://codereview.chromium.org/3329019/diff/40001/39013
File src/runtime.cc (right):

http://codereview.chromium.org/3329019/diff/40001/39013#newcode6282
src/runtime.cc:6282: static void SetInlineConstructStub(Handle<JSFunction>
function) {
renamed to TrySettingInlineConstructStub

On 2010/09/22 14:40:25, Vitaly wrote:
> This name is misleading. It makes the reader think an inline stub
> unconditionally set. Consider adding "IfPossible" suffix or "Maybe" prefix.

http://codereview.chromium.org/3329019/diff/40001/39013#newcode6380
src/runtime.cc:6380: // If the constructor isn't a proper function we throw a
type error.
I got confused when fuzz natives test failed on this function. Simplified.
On 2010/09/22 14:40:25, Vitaly wrote:
> Why do have to throw this specific error? Why can't CONVERT_ARG_CHECKED be
used
> here? If there is a reason, please update the comment.