Store transitioned JSArray maps in global context

Store transitioned JSArray maps in global context


BUG=
TEST=

Committed: https://code.google.com/p/v8/source/detail?r=10523

[danno, 2012-01-23 08:27:08]

PTAL.

Reviewer guidance:

Main changes are: 1) the transitioned ElementsKind for the default Array() are
cached in the global context and used in stubs in the Array constructor. 2)
Array construction in the runtime is bottlenecked through new common routines in
heap.cc to make sure that the ElementsKind transition policy for new Arrays is
handled consistently for all allocations.

[Jakob Kummerow, 2012-01-23 17:16:55]

LGTM with comments (mostly nits, but a few real issues too).

http://codereview.chromium.org/9073007/diff/26027/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:2899: // Use the transitioned cached
nit: += "map."

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode200
src/builtins.cc:200: // Initialize elements and length in case the transition
fails.
This comment seems misplaced.

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode769
src/builtins.cc:769: int result_len = final - k;
"= Max(final - k, 0)" please, as there's no guarantee that the difference is
>=0.

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode979
src/builtins.cc:979: if (!JSArray::cast(arg)->HasFastTypeElements()) {
This condition will never be true, as it's already checked for (and bailed out)
above (line 961). I think you mean:
if (JSArray::cast(arg)->HasFastElements()) {

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3735
src/heap.cc:3735: pretenure);
nit: can fit in one line

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3740
src/heap.cc:3740: ASSERT(length == 0);
nit: unnecessary, covered by ASSERT(capacity >= length) above.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3751
src/heap.cc:3751: if (!maybe_elms->To(&elms)) return maybe_elms;
This line occurs four times. You could move it out of the if-blocks, then you'd
need it only once.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3784
src/heap.cc:3784: pretenure);
nit: can fit in one line

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4511
src/heap.cc:4511: reinterpret_cast<FixedDoubleArray*>(elements_object);
While you're at it, you could replace these five lines with the following three:

FixedDoubleArray* elements;
MaybeObject* maybe_obj = AllocateRawFixedDoubleArray(length, pretenure);
if (!maybe_obj->To(&elements)) return maybe_obj;

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4525
src/heap.cc:4525: MaybeObject* maybe_obj = AllocateRawFixedDoubleArray(length,
pretenure);
Same suggestion here: three lines is better than five.
Also, if you s/Raw/Uninitialized/, you don't need to set map and length below.

http://codereview.chromium.org/9073007/diff/26027/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode810
src/heap.h:810: // Allocates a fixed double array with hole  values. Returns
nit: s/  / /

http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode2658
src/heap.h:2658: 
nit: No reason for an empty line here.

http://codereview.chromium.org/9073007/diff/26027/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:2188: // Use the transitioned cached
nit: += "map."

http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode1301
src/objects-inl.h:1301: if (current_map == global_context->smi_js_array_map()) {
What about the current_map == global_context->double_js_array_map() case? Not
frequent enough to be handled here?

http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode3920
src/objects-inl.h:3920: maybe_map = initial_map->CopyDropTransitions();
I'd s/initial_map/new_double_map/ here to make it more obvious what's going on.

http://codereview.chromium.org/9073007/diff/26027/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/objects.cc#newcode9666
src/objects.cc:9666: ElementsKind to_kind) {
nit: indentation was correct before

http://codereview.chromium.org/9073007/diff/26027/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/parser.cc#newcode4686
src/parser.cc:4686: elements,
nit: could put all three args onto the same line. Up to you.

http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc#newcode1
src/runtime.cc:1: j// Copyright 2012 the V8 project authors. All rights
reserved.
nit: superfluous "j" (does this compile?)

http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc#newcode518
src/runtime.cc:518: // If the ElementsKind of the constant values of the array
literal are less
This comment is confusing. Suggestion:
"Ensure that the boilerplate object has FAST_ELEMENTS, unless the flag is on or
the object is larger than the threshold."

http://codereview.chromium.org/9073007/diff/26027/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:4054: // Use the transitioned cached
nit: += "map."

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/array-construc...
File test/mjsunit/array-construct-transition.js (right):

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/array-construc...
test/mjsunit/array-construct-transition.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-kind.js
File test/mjsunit/elements-kind.js (right):

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-kind....
test/mjsunit/elements-kind.js:1: // Copyright 2011 the V8 project authors. All
rights reserved.
nit: 2012

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-trans...
File test/mjsunit/elements-transition.js (right):

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-trans...
test/mjsunit/elements-transition.js:1: // Copyright 2011 the V8 project authors.
All rights reserved.
nit: 2012

[danno, 2012-01-26 21:32:33]

feedback addressed, landing

http://codereview.chromium.org/9073007/diff/26027/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:2899: // Use the transitioned cached
On 2012/01/23 17:16:55, Jakob wrote:
> nit: += "map."

Done.

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode200
src/builtins.cc:200: // Initialize elements and length in case the transition
fails.
On 2012/01/23 17:16:55, Jakob wrote:
> This comment seems misplaced.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode769
src/builtins.cc:769: int result_len = final - k;
On 2012/01/23 17:16:55, Jakob wrote:
> "= Max(final - k, 0)" please, as there's no guarantee that the difference is
> >=0.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode979
src/builtins.cc:979: if (!JSArray::cast(arg)->HasFastTypeElements()) {
On 2012/01/23 17:16:55, Jakob wrote:
> This condition will never be true, as it's already checked for (and bailed
out)
> above (line 961). I think you mean:
> if (JSArray::cast(arg)->HasFastElements()) {

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3735
src/heap.cc:3735: pretenure);
On 2012/01/23 17:16:55, Jakob wrote:
> nit: can fit in one line

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3740
src/heap.cc:3740: ASSERT(length == 0);
On 2012/01/23 17:16:55, Jakob wrote:
> nit: unnecessary, covered by ASSERT(capacity >= length) above.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3751
src/heap.cc:3751: if (!maybe_elms->To(&elms)) return maybe_elms;
On 2012/01/23 17:16:55, Jakob wrote:
> This line occurs four times. You could move it out of the if-blocks, then
you'd
> need it only once.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3784
src/heap.cc:3784: pretenure);
On 2012/01/23 17:16:55, Jakob wrote:
> nit: can fit in one line

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4511
src/heap.cc:4511: reinterpret_cast<FixedDoubleArray*>(elements_object);
Unfortunately, you can't use FixedDoubleArray::cast here because the object
isn't well formed yet (it has no map).
On 2012/01/23 17:16:55, Jakob wrote:
> While you're at it, you could replace these five lines with the following
three:
> 
> FixedDoubleArray* elements;
> MaybeObject* maybe_obj = AllocateRawFixedDoubleArray(length, pretenure);
> if (!maybe_obj->To(&elements)) return maybe_obj;

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4525
src/heap.cc:4525: MaybeObject* maybe_obj = AllocateRawFixedDoubleArray(length,
pretenure);
Same here.
On 2012/01/23 17:16:55, Jakob wrote:
> Same suggestion here: three lines is better than five.
> Also, if you s/Raw/Uninitialized/, you don't need to set map and length below.

http://codereview.chromium.org/9073007/diff/26027/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode810
src/heap.h:810: // Allocates a fixed double array with hole  values. Returns
On 2012/01/23 17:16:55, Jakob wrote:
> nit: s/  / /

Done.

http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode2658
src/heap.h:2658: 
On 2012/01/23 17:16:55, Jakob wrote:
> nit: No reason for an empty line here.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:2188: // Use the transitioned cached
On 2012/01/23 17:16:55, Jakob wrote:
> nit: += "map."

Done.

http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode1301
src/objects-inl.h:1301: if (current_map == global_context->smi_js_array_map()) {
On 2012/01/23 17:16:55, Jakob wrote:
> What about the current_map == global_context->double_js_array_map() case? Not
> frequent enough to be handled here?

The SMI -> Object transition is so frequent and otherwise lightweight that it
makes sure to handle it here. Transitions involving double are less frequent.

http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode3920
src/objects-inl.h:3920: maybe_map = initial_map->CopyDropTransitions();
On 2012/01/23 17:16:55, Jakob wrote:
> I'd s/initial_map/new_double_map/ here to make it more obvious what's going
on.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/objects.cc#newcode9666
src/objects.cc:9666: ElementsKind to_kind) {
On 2012/01/23 17:16:55, Jakob wrote:
> nit: indentation was correct before

Done.

http://codereview.chromium.org/9073007/diff/26027/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/parser.cc#newcode4686
src/parser.cc:4686: elements,
On 2012/01/23 17:16:55, Jakob wrote:
> nit: could put all three args onto the same line. Up to you.

Done.

http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc#newcode1
src/runtime.cc:1: j// Copyright 2012 the V8 project authors. All rights
reserved.
On 2012/01/23 17:16:55, Jakob wrote:
> nit: superfluous "j" (does this compile?)

Done.

http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc#newcode518
src/runtime.cc:518: // If the ElementsKind of the constant values of the array
literal are less
On 2012/01/23 17:16:55, Jakob wrote:
> This comment is confusing. Suggestion:
> "Ensure that the boilerplate object has FAST_ELEMENTS, unless the flag is on
or
> the object is larger than the threshold."

Done.

http://codereview.chromium.org/9073007/diff/26027/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/9073007/diff/26027/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:4054: // Use the transitioned cached
On 2012/01/23 17:16:55, Jakob wrote:
> nit: += "map."

Done.

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/array-construc...
File test/mjsunit/array-construct-transition.js (right):

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/array-construc...
test/mjsunit/array-construct-transition.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/01/23 17:16:55, Jakob wrote:
> nit: 2012

Done.

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-kind.js
File test/mjsunit/elements-kind.js (right):

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-kind....
test/mjsunit/elements-kind.js:1: // Copyright 2011 the V8 project authors. All
rights reserved.
On 2012/01/23 17:16:55, Jakob wrote:
> nit: 2012

Done.

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-trans...
File test/mjsunit/elements-transition.js (right):

http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-trans...
test/mjsunit/elements-transition.js:1: // Copyright 2011 the V8 project authors.
All rights reserved.
On 2012/01/23 17:16:55, Jakob wrote:
> nit: 2012

Done.