Implement tracking and optimizations of packed arrays

Implement tracking and optimizations of packed arrays

R=jkummerow@chromium.org
TEST=jkummerow@chromium.org

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

[danno, 2012-05-08 15:39:15]

Please take a look. Only ia32 implementation is ready to go right now, I'm
following with the x64, ARM and MIPS code... please ignore them for now.

[Jakob Kummerow, 2012-05-13 21:55:27]

Some comments: a few real issues and a bunch of nits. These apply to patch set
6; I haven't looked at patch set 7 yet so maybe some of them are already
obsolete.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.cc
File src/elements-kind.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.c...
src/elements-kind.cc:55: ElementsKind **fast_elements_kind_sequence_ptr) {
nit: s/ **/** /

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.h
File src/elements-kind.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.h...
src/elements-kind.h:135: return !IsFastHoleyElementsKind(kind);
Let's add an "ASSERT(IsFastElementsKind(kind));" here. Just to prevent callers
from making wrong assumptions.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:502: virtual MaybeObject* SetCapacityAndLength(
nit: both this and the following change seem unnecessary.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:859: }
nit: indentation

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:865: JSObject* obj,
nit: unnecessary formatting change?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:889: 
nit: two empty lines (several more times below)

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:935: static MaybeObject* SetFastElementsCapacityAndLength(
nit: yet more formatting changes

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:974: FastPackedDoubleElementsAccessor,
nit: indentation (same for FastHoleyDoubleElementsAccessor)

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#new...
src/elements.cc:981: FastPackedDoubleElementsAccessor,
again

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.h
File src/elements.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.h#newc...
src/elements.h:87: virtual MaybeObject* SetCapacityAndLength(
nit: why this formatting change?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen-instru...
src/hydrogen-instructions.h:4370: if (original_map->has_fast_double_elements())
{
The bodies of these two if-blocks are the same; I'd suggest to merge them. IIUC,
the most appropriate condition is "if (original_map->has_fast_double_elements()
^ transitioned_map->has_fast_double_elements())", as a PACKED_DOUBLE ->
HOLEY_DOUBLE transition doesn't need to set those flags.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen.cc#new...
src/hydrogen.cc:5219: HInstruction* mapcheck = AddInstruction(new(zone())
nit: I'd break the line after "AddInstruction("

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/ic-ia32.cc...
src/ia32/ic-ia32.cc:892: // Value is a double. Transition FAST_*_SMI_ELEMENTS ->
FAST_*_DOUBLE_ELEMENTS
The comment says that FAST_*_SMI_ELEMENTS are transitioned, but
FAST_HOLEY_SMI_ELEMENTS actually jump to &slow. Is that intentional?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/ic-ia32.cc...
src/ia32/ic-ia32.cc:904: // Value is not a double, FAST_SMI_ONLY_ELEMENTS ->
FAST_ELEMENTS
nit: s/ONLY_// or s/ONLY_/*_/.
Also, HOLEY elements again aren't handled in the fast path. Intentional?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/macro-asse...
File src/ia32/macro-assembler-ia32.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.cc:507: current_map =
nit: fits on one line?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.cc:2188: ? FAST_HOLEY_ELEMENTS :
nit: I'd put the : under the ?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/stub-cache...
File src/ia32/stub-cache-ia32.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/stub-cache...
src/ia32/stub-cache-ia32.cc:1470: __
LoadTransitionedArrayMapConditional(FAST_HOLEY_SMI_ELEMENTS,
Didn't you mean to check for packed FAST_SMI_ELEMENTS here, and then for
FAST_HOLEY_SMI_ELEMENTS after "__ bind(&try_holey_map)"? I guess it doesn't
really make a difference, but the current naming is confusing.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#n...
src/objects-inl.h:1343: global_context->js_array_maps();
nit: fits on one line

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#n...
src/objects-inl.h:1678: ASSERT(index >= 0 && index < this->length());
nit: this ASSERT, while correct, is not necessary, as it's covered by the
"get(index)" call anyway. Feel free to leave it in if you prefer, though.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#n...
src/objects-inl.h:3954: heap->AllocateFixedArrayWithHoles(kElementsKindCount);
Do you mean kFastElementsKindCount here?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#n...
src/objects-inl.h:4807: (IsFastElementsKind(GetElementsKind()) &&
Did you mean IsFastSmiElementsKind?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc
File src/objects.cc (left):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#oldc...
src/objects.cc:9380: if (FLAG_trace_elements_transitions) {
I don't see why you would remove this.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:900: // Externalizing twice leaks the external resource, so its
prohibited by the
Why this change? Original version was correct.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:1066: ? 0 :
nit: I'd put the colon on the next line, directly below the question mark.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:2382: // This method is only called when safe_to_add_transition
has been found
nit: s/safe_to_add_transition/safe_to_add/

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:2450: IsFastObjectElementsKind(to_kind)) {
Wouldn't it make sense to check for any FAST kind here? Not sure if it would
ever happen in practice, but we don't want long chains of e.g. DICTIONARY <->
FAST_SMI maps either.

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:9417: array_length);
nit: why the line break?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:9450: ValidateElements();
Calls to ValidateElements() aren't wrapped in an #if DEBUG elsewhere (e.g. in
line 9420). Aren't they essentially no-ops in non-DEBUG mode anyway?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newc...
src/objects.cc:9588: ? SetFastDoubleElementsCapacityAndLength(new_length,
nit: why the line break?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newc...
src/runtime.cc:4745: FixedDoubleArray* double_array =
Why the line break?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newc...
src/runtime.cc:4761: FixedArray* object_array =
Why the line break?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newc...
src/runtime.cc:7021: RUNTIME_ASSERT(elements_array->HasFastObjectElements() ||
HasFastSmiOrObjectElements()?

https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newc...
src/runtime.cc:9943: JSObject::TransitionElementsKind(array, FAST_ELEMENTS));
s/FAST_ELEMENTS/to_kind/ !

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-...
File test/mjsunit/array-literal-transitions.js (left):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-...
test/mjsunit/array-literal-transitions.js:47: function array_literal_test() {
What happened here? Didn't like this test any more?

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-...
File test/mjsunit/array-literal-transitions.js (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-...
test/mjsunit/array-literal-transitions.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
nit: 2012

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elemen...
File test/mjsunit/elements-kind-depends.js (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elemen...
test/mjsunit/elements-kind-depends.js:49: //%OptimizeFunctionOnNextCall(burn);
This doesn't look intentional.

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elemen...
File test/mjsunit/elements-transition-hoisting.js (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elemen...
test/mjsunit/elements-transition-hoisting.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
nit: 2012

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/packed...
File test/mjsunit/packed-elements.js (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/packed...
test/mjsunit/packed-elements.js:28: // Flags: --allow-natives-syntax
--smi-only-arrays
Might want to specify --packed-arrays here too ;-)

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/regres...
File test/mjsunit/regress/regress-1849.js (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/regres...
test/mjsunit/regress/regress-1849.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
nit: 2012

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/regres...
test/mjsunit/regress/regress-1849.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=1878
I suspect s/1878/1849/ here.

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/unbox-...
File test/mjsunit/unbox-double-arrays.js (right):

https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/unbox-...
test/mjsunit/unbox-double-arrays.js:1: // Copyright 2011 the V8 project authors.
All rights reserved.
nit: 2012

[danno, 2012-05-22 11:05:20]

Please take another look.

http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.cc
File src/elements-kind.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.cc#newcode55
src/elements-kind.cc:55: ElementsKind **fast_elements_kind_sequence_ptr) {
On 2012/05/13 21:55:27, Jakob wrote:
> nit: s/ **/** /

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.h
File src/elements-kind.h (right):

http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.h#newcode135
src/elements-kind.h:135: return !IsFastHoleyElementsKind(kind);
On 2012/05/13 21:55:27, Jakob wrote:
> Let's add an "ASSERT(IsFastElementsKind(kind));" here. Just to prevent callers
> from making wrong assumptions.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode502
src/elements.cc:502: virtual MaybeObject* SetCapacityAndLength(
On 2012/05/13 21:55:27, Jakob wrote:
> nit: both this and the following change seem unnecessary.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode859
src/elements.cc:859: }
On 2012/05/13 21:55:27, Jakob wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode865
src/elements.cc:865: JSObject* obj,
On 2012/05/13 21:55:27, Jakob wrote:
> nit: unnecessary formatting change?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode889
src/elements.cc:889: 
On 2012/05/13 21:55:27, Jakob wrote:
> nit: two empty lines (several more times below)

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode935
src/elements.cc:935: static MaybeObject* SetFastElementsCapacityAndLength(
On 2012/05/13 21:55:27, Jakob wrote:
> nit: yet more formatting changes

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode974
src/elements.cc:974: FastPackedDoubleElementsAccessor,
On 2012/05/13 21:55:27, Jakob wrote:
> nit: indentation (same for FastHoleyDoubleElementsAccessor)

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode981
src/elements.cc:981: FastPackedDoubleElementsAccessor,
On 2012/05/13 21:55:27, Jakob wrote:
> again

Done.

http://codereview.chromium.org/10170030/diff/9002/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/10170030/diff/9002/src/elements.h#newcode87
src/elements.h:87: virtual MaybeObject* SetCapacityAndLength(
On 2012/05/13 21:55:27, Jakob wrote:
> nit: why this formatting change?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/10170030/diff/9002/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:4370: if (original_map->has_fast_double_elements())
{
On 2012/05/13 21:55:27, Jakob wrote:
> The bodies of these two if-blocks are the same; I'd suggest to merge them.
IIUC,
> the most appropriate condition is "if
(original_map->has_fast_double_elements()
> ^ transitioned_map->has_fast_double_elements())", as a PACKED_DOUBLE ->
> HOLEY_DOUBLE transition doesn't need to set those flags.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/hydrogen.cc#newcode5219
src/hydrogen.cc:5219: HInstruction* mapcheck = AddInstruction(new(zone())
On 2012/05/13 21:55:27, Jakob wrote:
> nit: I'd break the line after "AddInstruction("

Done.

http://codereview.chromium.org/10170030/diff/9002/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/ia32/ic-ia32.cc#newcode892
src/ia32/ic-ia32.cc:892: // Value is a double. Transition FAST_*_SMI_ELEMENTS ->
FAST_*_DOUBLE_ELEMENTS
For now, yes. When Toon finishes his changes, all cases will be handled here. 
On 2012/05/13 21:55:27, Jakob wrote:
> The comment says that FAST_*_SMI_ELEMENTS are transitioned, but
> FAST_HOLEY_SMI_ELEMENTS actually jump to &slow. Is that intentional?

http://codereview.chromium.org/10170030/diff/9002/src/ia32/ic-ia32.cc#newcode904
src/ia32/ic-ia32.cc:904: // Value is not a double, FAST_SMI_ONLY_ELEMENTS ->
FAST_ELEMENTS
On 2012/05/13 21:55:27, Jakob wrote:
> nit: s/ONLY_// or s/ONLY_/*_/.
> Also, HOLEY elements again aren't handled in the fast path. Intentional?

Done.

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

http://codereview.chromium.org/10170030/diff/9002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:507: current_map =
On 2012/05/13 21:55:27, Jakob wrote:
> nit: fits on one line?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:2188: ? FAST_HOLEY_ELEMENTS :
On 2012/05/13 21:55:27, Jakob wrote:
> nit: I'd put the : under the ?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1470: __
LoadTransitionedArrayMapConditional(FAST_HOLEY_SMI_ELEMENTS,
On 2012/05/13 21:55:27, Jakob wrote:
> Didn't you mean to check for packed FAST_SMI_ELEMENTS here, and then for
> FAST_HOLEY_SMI_ELEMENTS after "__ bind(&try_holey_map)"? I guess it doesn't
> really make a difference, but the current naming is confusing.

Done.

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

http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode1343
src/objects-inl.h:1343: global_context->js_array_maps();
On 2012/05/13 21:55:27, Jakob wrote:
> nit: fits on one line

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode1678
src/objects-inl.h:1678: ASSERT(index >= 0 && index < this->length());
On 2012/05/13 21:55:27, Jakob wrote:
> nit: this ASSERT, while correct, is not necessary, as it's covered by the
> "get(index)" call anyway. Feel free to leave it in if you prefer, though.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode3954
src/objects-inl.h:3954: heap->AllocateFixedArrayWithHoles(kElementsKindCount);
On 2012/05/13 21:55:27, Jakob wrote:
> Do you mean kFastElementsKindCount here?

kElementsKindCount is better here, since it removes the need to implement
special cases when testing against DICTIONARY and other maps. In this array,
they are initialize with holes, so comparisons always fail without having to
gate code elsewhere with IsFastElementKind() checks before looking into the map
array.

http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode4807
src/objects-inl.h:4807: (IsFastElementsKind(GetElementsKind()) &&
On 2012/05/13 21:55:27, Jakob wrote:
> Did you mean IsFastSmiElementsKind?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#oldcode9380
src/objects.cc:9380: if (FLAG_trace_elements_transitions) {
Because it's a dupe of the transition printing done in
SetFastElementsCapacityAndLength if it actually changes ElementsKind. 
On 2012/05/13 21:55:27, Jakob wrote:
> I don't see why you would remove this.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode900
src/objects.cc:900: // Externalizing twice leaks the external resource, so its
prohibited by the
On 2012/05/13 21:55:27, Jakob wrote:
> Why this change? Original version was correct.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode1066
src/objects.cc:1066: ? 0 :
On 2012/05/13 21:55:27, Jakob wrote:
> nit: I'd put the colon on the next line, directly below the question mark.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode2382
src/objects.cc:2382: // This method is only called when safe_to_add_transition
has been found
On 2012/05/13 21:55:27, Jakob wrote:
> nit: s/safe_to_add_transition/safe_to_add/

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode2450
src/objects.cc:2450: IsFastObjectElementsKind(to_kind)) {
On 2012/05/13 21:55:27, Jakob wrote:
> Wouldn't it make sense to check for any FAST kind here? Not sure if it would
> ever happen in practice, but we don't want long chains of e.g. DICTIONARY <->
> FAST_SMI maps either.

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode9417
src/objects.cc:9417: array_length);
On 2012/05/13 21:55:27, Jakob wrote:
> nit: why the line break?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode9450
src/objects.cc:9450: ValidateElements();
On 2012/05/13 21:55:27, Jakob wrote:
> Calls to ValidateElements() aren't wrapped in an #if DEBUG elsewhere (e.g. in
> line 9420). Aren't they essentially no-ops in non-DEBUG mode anyway?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode9588
src/objects.cc:9588: ? SetFastDoubleElementsCapacityAndLength(new_length,
On 2012/05/13 21:55:27, Jakob wrote:
> nit: why the line break?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode4745
src/runtime.cc:4745: FixedDoubleArray* double_array =
On 2012/05/13 21:55:27, Jakob wrote:
> Why the line break?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode4761
src/runtime.cc:4761: FixedArray* object_array =
On 2012/05/13 21:55:27, Jakob wrote:
> Why the line break?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode7021
src/runtime.cc:7021: RUNTIME_ASSERT(elements_array->HasFastObjectElements() ||
On 2012/05/13 21:55:27, Jakob wrote:
> HasFastSmiOrObjectElements()?

Done.

http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode9943
src/runtime.cc:9943: JSObject::TransitionElementsKind(array, FAST_ELEMENTS));
On 2012/05/13 21:55:27, Jakob wrote:
> s/FAST_ELEMENTS/to_kind/ !

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-...
File test/mjsunit/array-literal-transitions.js (left):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-...
test/mjsunit/array-literal-transitions.js:47: function array_literal_test() {
On 2012/05/13 21:55:27, Jakob wrote:
> What happened here? Didn't like this test any more?

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-...
File test/mjsunit/array-literal-transitions.js (right):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-...
test/mjsunit/array-literal-transitions.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
> nit: 2012

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-kind-...
File test/mjsunit/elements-kind-depends.js (right):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-kind-...
test/mjsunit/elements-kind-depends.js:49: //%OptimizeFunctionOnNextCall(burn);
On 2012/05/13 21:55:27, Jakob wrote:
> This doesn't look intentional.

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-trans...
File test/mjsunit/elements-transition-hoisting.js (right):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-trans...
test/mjsunit/elements-transition-hoisting.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
> nit: 2012

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/packed-element...
File test/mjsunit/packed-elements.js (right):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/packed-element...
test/mjsunit/packed-elements.js:28: // Flags: --allow-natives-syntax
--smi-only-arrays
On 2012/05/13 21:55:27, Jakob wrote:
> Might want to specify --packed-arrays here too ;-)

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-1849.js (right):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-1849.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
> nit: 2012

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-1849.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=1878
On 2012/05/13 21:55:27, Jakob wrote:
> I suspect s/1878/1849/ here.

Done.

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/unbox-double-a...
File test/mjsunit/unbox-double-arrays.js (right):

http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/unbox-double-a...
test/mjsunit/unbox-double-arrays.js:1: // Copyright 2011 the V8 project authors.
All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
> nit: 2012

Done.

[Jakob Kummerow, 2012-05-22 17:36:49]

Finally, the moment you've been waiting for for a long, long time...

*drumroll*

LGTM WITH NITS!

:-)

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/arm/lithium-co...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:3915: IsFastElementsKind(to_kind)) {
IsFastObjectElementsKind(to_kind) ?

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/codegen.h
File src/codegen.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/codegen.h#newc...
src/codegen.h:98: static void GenerateMapChangeElementTransition(MacroAssembler*
masm);
nit: for naming consistency, I'd add an 's':
Element*s*TransitionGenerator::GenerateMapChangeElement*s*Transition

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/elements-kind.h
File src/elements-kind.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/elements-kind....
src/elements-kind.h:136: !IsHoleyElementsKind(kind);
nit: IsFastHoleyElementsKind would be ever so slightly faster.

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/flag-definitio...
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/flag-definitio...
src/flag-definitions.h:153: DEFINE_bool(packed_arrays, true, "optimizes arrays
that have no holes")
intentional?

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:3572: IsFastElementsKind(to_kind)) {
IsFastObjectElementsKind(to_kind) ?

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/mips/lithium-c...
File src/mips/lithium-codegen-mips.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/mips/lithium-c...
src/mips/lithium-codegen-mips.cc:3705: IsFastElementsKind(to_kind)) {
IsFastObjectElementsKind(to_kind) ?

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/mips/lithium-c...
src/mips/lithium-codegen-mips.cc:4461: if (boilerplate_elements_kind !=
TERMINAL_FAST_ELEMENTS_KIND) {
Other arches ask "if
(CanTransitionToMoreGeneralFastElementsKind(boilerplate_elements_kind, true))"
here

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/objects-inl.h#...
src/objects-inl.h:4857: (IsFastSmiElementsKind(GetElementsKind()) &&
nit: could just use IsFastSmiOrObjectElementsKind

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/lithium-co...
File src/x64/lithium-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/lithium-co...
src/x64/lithium-codegen-x64.cc:3515: IsFastElementsKind(to_kind)) {
IsFastObjectElementsKind(to_kind) ?

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/lithium-co...
src/x64/lithium-codegen-x64.cc:4339: // already been converted to FAST_ELEMENTS.
nit: s/FAST_ELEMENTS/TERMINAL_FAST_ELEMENTS_KIND/

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/lithium-x6...
File src/x64/lithium-x64.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/lithium-x6...
src/x64/lithium-x64.cc:2017: bool simple_map_change =
(GetHoleyElementsKind(from_kind) == to_kind) ||
This computation is used often enough that it would be nice to factor it out
into elements-kind.h:

bool IsSimpleMapChange(ElementsKind from_kind, ElementsKind to_kind) {
  return (GetHoleyElementsKind(from_kind) == to_kind) ||
      (IsFastSmiElementsKind(from_kind) &&
      IsFastObjectElementsKind(to_kind));
}

But then again, this CL is large enough as it is, so feel free to postpone such
cleanup.

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/stub-cache...
File src/x64/stub-cache-x64.cc (right):

https://chromiumcodereview.appspot.com/10170030/diff/18004/src/x64/stub-cache...
src/x64/stub-cache-x64.cc:1452: __ movq(rdi, FieldOperand(rdx,
JSArray::kElementsOffset));
You have rdi's original value in r9, can use that (or remove line 1440 and copy
this (1452) to 1463).

[danno, 2012-05-23 14:25:36]

Feedback addressed. Landing.

http://codereview.chromium.org/10170030/diff/18004/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10170030/diff/18004/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3915: IsFastElementsKind(to_kind)) {
On 2012/05/22 17:36:49, Jakob wrote:
> IsFastObjectElementsKind(to_kind) ?

Done.

http://codereview.chromium.org/10170030/diff/18004/src/codegen.h
File src/codegen.h (right):

http://codereview.chromium.org/10170030/diff/18004/src/codegen.h#newcode98
src/codegen.h:98: static void GenerateMapChangeElementTransition(MacroAssembler*
masm);
On 2012/05/22 17:36:49, Jakob wrote:
> nit: for naming consistency, I'd add an 's':
> Element*s*TransitionGenerator::GenerateMapChangeElement*s*Transition

Done.

http://codereview.chromium.org/10170030/diff/18004/src/elements-kind.h
File src/elements-kind.h (right):

http://codereview.chromium.org/10170030/diff/18004/src/elements-kind.h#newcod...
src/elements-kind.h:136: !IsHoleyElementsKind(kind);
On 2012/05/22 17:36:49, Jakob wrote:
> nit: IsFastHoleyElementsKind would be ever so slightly faster.

Done.

http://codereview.chromium.org/10170030/diff/18004/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/10170030/diff/18004/src/flag-definitions.h#new...
src/flag-definitions.h:153: DEFINE_bool(packed_arrays, true, "optimizes arrays
that have no holes")
On 2012/05/22 17:36:49, Jakob wrote:
> intentional?

Done.

http://codereview.chromium.org/10170030/diff/18004/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/10170030/diff/18004/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:3572: IsFastElementsKind(to_kind)) {
On 2012/05/22 17:36:49, Jakob wrote:
> IsFastObjectElementsKind(to_kind) ?

Done.

http://codereview.chromium.org/10170030/diff/18004/src/mips/lithium-codegen-m...
File src/mips/lithium-codegen-mips.cc (right):

http://codereview.chromium.org/10170030/diff/18004/src/mips/lithium-codegen-m...
src/mips/lithium-codegen-mips.cc:3705: IsFastElementsKind(to_kind)) {
On 2012/05/22 17:36:49, Jakob wrote:
> IsFastObjectElementsKind(to_kind) ?

Done.

http://codereview.chromium.org/10170030/diff/18004/src/mips/lithium-codegen-m...
src/mips/lithium-codegen-mips.cc:4461: if (boilerplate_elements_kind !=
TERMINAL_FAST_ELEMENTS_KIND) {
On 2012/05/22 17:36:49, Jakob wrote:
> Other arches ask "if
> (CanTransitionToMoreGeneralFastElementsKind(boilerplate_elements_kind, true))"
> here

Done.

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

http://codereview.chromium.org/10170030/diff/18004/src/objects-inl.h#newcode4857
src/objects-inl.h:4857: (IsFastSmiElementsKind(GetElementsKind()) &&
That's a little bit different. The () make sure that if it's Smi, then it can
contain only Smis or Holes.
On 2012/05/22 17:36:49, Jakob wrote:
> nit: could just use IsFastSmiOrObjectElementsKind

http://codereview.chromium.org/10170030/diff/18004/src/x64/lithium-codegen-x6...
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/10170030/diff/18004/src/x64/lithium-codegen-x6...
src/x64/lithium-codegen-x64.cc:3515: IsFastElementsKind(to_kind)) {
On 2012/05/22 17:36:49, Jakob wrote:
> IsFastObjectElementsKind(to_kind) ?

Done.

http://codereview.chromium.org/10170030/diff/18004/src/x64/lithium-codegen-x6...
src/x64/lithium-codegen-x64.cc:4339: // already been converted to FAST_ELEMENTS.
On 2012/05/22 17:36:49, Jakob wrote:
> nit: s/FAST_ELEMENTS/TERMINAL_FAST_ELEMENTS_KIND/

Done.

http://codereview.chromium.org/10170030/diff/18004/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

http://codereview.chromium.org/10170030/diff/18004/src/x64/lithium-x64.cc#new...
src/x64/lithium-x64.cc:2017: bool simple_map_change =
(GetHoleyElementsKind(from_kind) == to_kind) ||
On 2012/05/22 17:36:49, Jakob wrote:
> This computation is used often enough that it would be nice to factor it out
> into elements-kind.h:
> 
> bool IsSimpleMapChange(ElementsKind from_kind, ElementsKind to_kind) {
>   return (GetHoleyElementsKind(from_kind) == to_kind) ||
>       (IsFastSmiElementsKind(from_kind) &&
>       IsFastObjectElementsKind(to_kind));
> }
> 
> But then again, this CL is large enough as it is, so feel free to postpone
such
> cleanup.

Done.

http://codereview.chromium.org/10170030/diff/18004/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/10170030/diff/18004/src/x64/stub-cache-x64.cc#...
src/x64/stub-cache-x64.cc:1452: __ movq(rdi, FieldOperand(rdx,
JSArray::kElementsOffset));
On 2012/05/22 17:36:49, Jakob wrote:
> You have rdi's original value in r9, can use that (or remove line 1440 and
copy
> this (1452) to 1463).

Done.