Fix array concat to follow the specification in the presence of element getters.

Fix array concat to follow the specification in the presence of element getters.

Also fix issue 1175 and 1177.

BUG=v8:1175
Committed: http://code.google.com/p/v8/source/detail?r=6934

[Erik Corry, 2011-02-24 10:36:56]

LGTM

http://codereview.chromium.org/6568007/diff/1/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/6568007/diff/1/src/handles.h#newcode101
src/handles.h:101: class RootVar {
I think I would prefer MutableHandle as a name with a note to the effect that
they don't share state with Handles or other MutableHandles.

http://codereview.chromium.org/6568007/diff/1/src/handles.h#newcode117
src/handles.h:117: INLINE(void operator=(Handle<T> value)) {
Would it be premature generalization to provide the inverse operation too?

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

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8051
src/runtime.cc:8051: memcpy(new_array->data_start(),
Seems like there is a missing write barrier here.  Lets just go to slow case
right away.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8101
src/runtime.cc:8101: // Add 25% for good measure.
HashTable<Shape, Key>::Allocate already rounds up to the nearest power of 2, but
that is not enough for you?  EnsureCapacity wants to have 50% free.  Unless you
have a benchmark that shows a need I suggest just passing the capacity that you
have and letting the internal implementation of the dictionary take care of it.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8102
src/runtime.cc:8102: int capacity = current_length + (current_length >> 2);
If there is any point to the uint32_t then this should surely also be a uint32_t
too?  I would prefer just int or unsigned throughout tbh.  The exact number of
bits is not important here.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8111
src/runtime.cc:8111: if (*result != *slow_storage) {
It seems like there's not much point to the if here.  It is doing almost all the
same work as the assignment in order to determine whether or not to do an
assignment.  The only point might be to put an UNREACHABLE() in the 'if' and use
that to check that you have test coverage for the use of your new Handle class.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8128
src/runtime.cc:8128: static uint32_t EstimateElementCount(Handle<JSArray> array)
{
Lots of use of unsigned and fixed bit-width types here and casts caused by it. 
Can we just use int?

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8160
src/runtime.cc:8160: // intherited elements.
intherited -> inherited

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8179
src/runtime.cc:8179: Handle<Smi>
e(Smi::FromInt(static_cast<int>(array->get(j))));
Not something introduced by you, but don't we have an unbounded number of
Handles in a HandleScope here?

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8365
src/runtime.cc:8365: } while (j < n && indices[j] == index);
So if we find a dupe shouldn't we update the length somewhere?

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8423
src/runtime.cc:8423: * TODO(lrn): Fix non-compliance for very large
concatenations and update to
Is there a bug number for this one?

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8437
src/runtime.cc:8437: // that mutate other arguments (but will otherwise be
precise).
Parenthetic remark not accurate in the presence of inherited elements.

[Lasse Reichstein, 2011-02-24 13:44:41]

http://codereview.chromium.org/6568007/diff/1/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/6568007/diff/1/src/handles.h#newcode101
src/handles.h:101: class RootVar {
Renamed to HandleCell.
The name MutableHandle seems like it should be a subclass of Handle, which it's
not.

http://codereview.chromium.org/6568007/diff/1/src/handles.h#newcode117
src/handles.h:117: INLINE(void operator=(Handle<T> value)) {
I would prefer only having explicit ways to extract the value of the HandleCell,
in this case we already have ToHandle.

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

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8051
src/runtime.cc:8051: memcpy(new_array->data_start(),
True. This overflow can only happen if an array change length during iteration.
That's bad enough that going directly to Dictionary mode is warranted. Done.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8101
src/runtime.cc:8101: // Add 25% for good measure.
I'll remove the extra 25% here, and later where we do the same thing.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8102
src/runtime.cc:8102: int capacity = current_length + (current_length >> 2);
Then it will be int here. When used as an array element index, the number of
bits is relevant.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8111
src/runtime.cc:8111: if (*result != *slow_storage) {
Will remove the if.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8128
src/runtime.cc:8128: static uint32_t EstimateElementCount(Handle<JSArray> array)
{
Not when the value is used as an array element index or array length. They
actually use the entire range of uint32_t.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8179
src/runtime.cc:8179: Handle<Smi>
e(Smi::FromInt(static_cast<int>(array->get(j))));
Good catch.
Before I changed the visitor to hold the storage in a HandleCell, we couldn't
have local handle-scopes. If a dictionary grew, the new dictionary would be in a
handle from the innermost handle-scope, and would be thrown away when we exited
it.

HandleScope added here and below.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8365
src/runtime.cc:8365: } while (j < n && indices[j] == index);
No, a dupe in the indices means that we have an inherited element that is
shadowed by another element.
That doesn't affect the length at all.

The case where our initial estimates were off is the count of elements, if we
have inherited elements that are not shadowed. Duplicate indices here is
actually benign.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8423
src/runtime.cc:8423: * TODO(lrn): Fix non-compliance for very large
concatenations and update to
There is: 581.
Changing comment to match.

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8437
src/runtime.cc:8437: // that mutate other arguments (but will otherwise be
precise).
The length is indifferent to inherited elements. We use the arrays's "length"
property, not its elements, to determine the size it takes up in the
concatenated result.

[Lasse Reichstein, 2011-02-24 13:45:49]

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

http://codereview.chromium.org/6568007/diff/1/src/runtime.cc#newcode8160
src/runtime.cc:8160: // intherited elements.
Fixed.