Allocate as many object-literal properties as possible inobject....

Allocate as many object-literal properties as possible inobject.

This can lead to large objects which wastes a lot of space if we normalize properties.  We therfore clear the inobject properties when normalizing properties.  This is done by adjusting the instance size in the new map and overwriting the inobject properties with a filler.

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

[Kasper Lund, 2009-01-09 13:45:23]

LGTM, but I think you should extend the test case to make sure the normalization
code has been tested and that the filler objects can be GC'ed correctly.

http://codereview.chromium.org/17308/diff/213/17
File src/objects.cc (right):

http://codereview.chromium.org/17308/diff/213/17#newcode1919
Line 1919: Object* filler_map = Heap::AllocateMap(FILLER_TYPE,
instance_size_delta);
It seems a bit nasty to have to allocate a new map for the filler object. How
about just using a fixed array if it fits (at least two words) or a simple
one-word filler otherwise?

http://codereview.chromium.org/17308/diff/213/18
File src/runtime.cc (right):

http://codereview.chromium.org/17308/diff/213/18#newcode105
Line 105: int number_of_properties = constant_properties->length()/2;
Spaces around / operator?

http://codereview.chromium.org/17308/diff/213/14
File test/mjsunit/large-object-literal.js (right):

http://codereview.chromium.org/17308/diff/213/14#newcode29
Line 29: var nofProperties = 256;
How about refactoring this test code so that nofProperties is a parameter to a
function and try it with different values like 1,2,5,35,150,255,256,280 and
maybe extending it to force the object into normal case and also force a GC to
make sure everything works with the normalization before checking the property
values?

[Dean McNamee, 2009-01-09 14:29:00]

Before:
 ["FIXED_ARRAY_TYPE", 3238656],

After:
 ["FIXED_ARRAY_TYPE", 216438],

More than 10x less FIXED_ARRAY_TYPEs were GC'd in my test code.

On 2009/01/09 13:45:23, Kasper Lund wrote:
> LGTM, but I think you should extend the test case to make sure the
normalization
> code has been tested and that the filler objects can be GC'ed correctly.
> 
> http://codereview.chromium.org/17308/diff/213/17
> File src/objects.cc (right):
> 
> http://codereview.chromium.org/17308/diff/213/17#newcode1919
> Line 1919: Object* filler_map = Heap::AllocateMap(FILLER_TYPE,
> instance_size_delta);
> It seems a bit nasty to have to allocate a new map for the filler object. How
> about just using a fixed array if it fits (at least two words) or a simple
> one-word filler otherwise?
> 
> http://codereview.chromium.org/17308/diff/213/18
> File src/runtime.cc (right):
> 
> http://codereview.chromium.org/17308/diff/213/18#newcode105
> Line 105: int number_of_properties = constant_properties->length()/2;
> Spaces around / operator?
> 
> http://codereview.chromium.org/17308/diff/213/14
> File test/mjsunit/large-object-literal.js (right):
> 
> http://codereview.chromium.org/17308/diff/213/14#newcode29
> Line 29: var nofProperties = 256;
> How about refactoring this test code so that nofProperties is a parameter to a
> function and try it with different values like 1,2,5,35,150,255,256,280 and
> maybe extending it to force the object into normal case and also force a GC to
> make sure everything works with the normalization before checking the property
> values?

[Dean McNamee, 2009-01-09 15:56:31]

Performance improvements, 17s vs 19s.

On 2009/01/09 14:29:00, Dean McNamee wrote:
> Before:
>  ["FIXED_ARRAY_TYPE", 3238656],
> 
> After:
>  ["FIXED_ARRAY_TYPE", 216438],
> 
> More than 10x less FIXED_ARRAY_TYPEs were GC'd in my test code.
> 
> On 2009/01/09 13:45:23, Kasper Lund wrote:
> > LGTM, but I think you should extend the test case to make sure the
> normalization
> > code has been tested and that the filler objects can be GC'ed correctly.
> > 
> > http://codereview.chromium.org/17308/diff/213/17
> > File src/objects.cc (right):
> > 
> > http://codereview.chromium.org/17308/diff/213/17#newcode1919
> > Line 1919: Object* filler_map = Heap::AllocateMap(FILLER_TYPE,
> > instance_size_delta);
> > It seems a bit nasty to have to allocate a new map for the filler object.
How
> > about just using a fixed array if it fits (at least two words) or a simple
> > one-word filler otherwise?
> > 
> > http://codereview.chromium.org/17308/diff/213/18
> > File src/runtime.cc (right):
> > 
> > http://codereview.chromium.org/17308/diff/213/18#newcode105
> > Line 105: int number_of_properties = constant_properties->length()/2;
> > Spaces around / operator?
> > 
> > http://codereview.chromium.org/17308/diff/213/14
> > File test/mjsunit/large-object-literal.js (right):
> > 
> > http://codereview.chromium.org/17308/diff/213/14#newcode29
> > Line 29: var nofProperties = 256;
> > How about refactoring this test code so that nofProperties is a parameter to
a
> > function and try it with different values like 1,2,5,35,150,255,256,280 and
> > maybe extending it to force the object into normal case and also force a GC
to
> > make sure everything works with the normalization before checking the
property
> > values?

[Mads Ager (chromium), 2009-01-12 10:47:32]

On 2009/01/09 13:45:23, Kasper Lund wrote:
> LGTM, but I think you should extend the test case to make sure the
normalization
> code has been tested and that the filler objects can be GC'ed correctly.

Thanks!  I agree with your comments and have followed your suggestions.

[Dean McNamee, 2009-01-12 11:04:00]

http://codereview.chromium.org/17308/diff/807/814
File src/objects.h (right):

http://codereview.chromium.org/17308/diff/807/814#newcode174
Line 174: // PropertyNormalizationMode is used to specify wheter or not to
whether, (you can also drop the or not)