Use proper ToLength() operation in %ArrayConcat()

Use proper ToLength() operation in %ArrayConcat()

LOG=N
R=dslomov@chromium.org
BUG=

Committed: https://crrev.com/aed5d734e66093c54caedf957895764a20db82b1
Cr-Commit-Position: refs/heads/master@{#25828}

[caitp (gmail), 2014-12-13 02:00:47]

https://codereview.chromium.org/799853003/diff/1/src/runtime/runtime-array.cc
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799853003/diff/1/src/runtime/runtime-array.cc...
src/runtime/runtime-array.cc:464: // TODO(caitp): ToLength(Get(receiver,
"length")) should be a single op.
I'm not sure how big of a problem this is, but it seems simpler to just call the
JS implementation of ToLength() rather than write it in C++. The perf-impact
should only affect uses of Symbol.isConcatSpreadable, so it probably won't
matter much in most cases.

[Dmitry Lomov (no reviews), 2014-12-13 10:42:48]

dslomov@chromium.org changed reviewers:
+ dslomov@chromium.org

[Dmitry Lomov (no reviews), 2014-12-13 10:42:49]

Other than comment below looks good, but I think one more important thing to
look at is proto chain walk in concat

https://codereview.chromium.org/799853003/diff/1/src/runtime/runtime-array.cc
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799853003/diff/1/src/runtime/runtime-array.cc...
src/runtime/runtime-array.cc:464: // TODO(caitp): ToLength(Get(receiver,
"length")) should be a single op.
On 2014/12/13 02:00:47, caitp wrote:
> I'm not sure how big of a problem this is, but it seems simpler to just call
the
> JS implementation of ToLength() rather than write it in C++. The perf-impact
> should only affect uses of Symbol.isConcatSpreadable, so it probably won't
> matter much in most cases.

You should handle simple cases in C++ here (e.g. when val is already a number,
just convert directly without calling into JS)

[caitp (gmail), 2014-12-13 16:41:34]

On 2014/12/13 10:42:49, Dmitry Lomov (chromium) wrote:
> Other than comment below looks good, but I think one more important thing to
> look at is proto chain walk in concat

Interestingly, for (simple) arrays, data properties in the prototype chain seem
to work perfectly well, but accessor properties do not --- different from
SpiderMonkey. Haven't tested this for more interesting element types, but it
seems like an issue that probably should be showing up in ES5's test262. I'll
send another CL for those.

> https://codereview.chromium.org/799853003/diff/1/src/runtime/runtime-array.cc
> File src/runtime/runtime-array.cc (right):
> 
>
https://codereview.chromium.org/799853003/diff/1/src/runtime/runtime-array.cc...
> src/runtime/runtime-array.cc:464: // TODO(caitp): ToLength(Get(receiver,
> "length")) should be a single op.
> On 2014/12/13 02:00:47, caitp wrote:
> > I'm not sure how big of a problem this is, but it seems simpler to just call
> the
> > JS implementation of ToLength() rather than write it in C++. The perf-impact
> > should only affect uses of Symbol.isConcatSpreadable, so it probably won't
> > matter much in most cases.
> 
> You should handle simple cases in C++ here (e.g. when val is already a number,
> just convert directly without calling into JS)

Done.. This is still slow if length is a negative value, and incorrect if length
> 2^32, but I'm a bit iffy about adding support for big ints just yet.

[Dmitry Lomov (no reviews), 2014-12-15 07:23:52]

Couple more comments (one thing I missed in my prev review is that ToUint32 can
fail leaving length uninitialized)

https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
src/runtime/runtime-array.cc:468: val->ToUint32(&length);
This is not good. 
If ToUInt32 returns false, `length` is uninitialized, but you proceed as normal.
No idea what the code below would do.
It is ok to not support large lengths, but fail gracefully, do not do random
things.

https://codereview.chromium.org/799853003/diff/20001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-concat.js (right):

https://codereview.chromium.org/799853003/diff/20001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-concat.js:80: "length": {valueOf: null, toString:
null},
Add a test where `toString` and `valueOf` throw an exception and validate that
the exception you get out of `concat` is exact same exception they throw.

[caitp (gmail), 2014-12-15 14:11:00]

https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
src/runtime/runtime-array.cc:468: val->ToUint32(&length);
On 2014/12/15 07:23:51, Dmitry Lomov (chromium) wrote:
> This is not good. 
> If ToUInt32 returns false, `length` is uninitialized, but you proceed as
normal.
> No idea what the code below would do.
> It is ok to not support large lengths, but fail gracefully, do not do random
> things. 

I don't think it can return false here, because `val` is (I think) guaranteed to
be a number here. In either case, `length` is initialized to 0 before any of
this happens, so if it does return false and doesn't initialize `length`, it's a
zero value.

[Dmitry Lomov (no reviews), 2014-12-15 14:18:20]

https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
src/runtime/runtime-array.cc:468: val->ToUint32(&length);
On 2014/12/15 14:11:00, caitp wrote:
> On 2014/12/15 07:23:51, Dmitry Lomov (chromium) wrote:
> > This is not good. 
> > If ToUInt32 returns false, `length` is uninitialized, but you proceed as
> normal.
> > No idea what the code below would do.
> > It is ok to not support large lengths, but fail gracefully, do not do random
> > things. 
> 
> I don't think it can return false here, because `val` is (I think) guaranteed
to
> be a number here. In either case, `length` is initialized to 0 before any of
> this happens, so if it does return false and doesn't initialize `length`, it's
a
> zero value.
No, ToString calls user code, so anything can happen.
Good point about length 0 though.

[caitp (gmail), 2014-12-15 17:46:19]

On 2014/12/15 14:18:20, Dmitry Lomov (chromium) wrote:
>
https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
> File src/runtime/runtime-array.cc (right):
> 
>
https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
> src/runtime/runtime-array.cc:468: val->ToUint32(&length);
> On 2014/12/15 14:11:00, caitp wrote:
> > On 2014/12/15 07:23:51, Dmitry Lomov (chromium) wrote:
> > > This is not good. 
> > > If ToUInt32 returns false, `length` is uninitialized, but you proceed as
> > normal.
> > > No idea what the code below would do.
> > > It is ok to not support large lengths, but fail gracefully, do not do
random
> > > things. 
> > 
> > I don't think it can return false here, because `val` is (I think)
guaranteed
> to
> > be a number here. In either case, `length` is initialized to 0 before any of
> > this happens, so if it does return false and doesn't initialize `length`,
it's
> a
> > zero value.
> No, ToString calls user code, so anything can happen.
> Good point about length 0 though.

Okay, I'll add some test cases where toString / valueOf return some primitive
non-numbery things --- other than that it looks ok?

[Dmitry Lomov (no reviews), 2014-12-15 17:48:32]

On 2014/12/15 17:46:19, caitp wrote:
> On 2014/12/15 14:18:20, Dmitry Lomov (chromium) wrote:
> >
>
https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
> > File src/runtime/runtime-array.cc (right):
> > 
> >
>
https://codereview.chromium.org/799853003/diff/20001/src/runtime/runtime-arra...
> > src/runtime/runtime-array.cc:468: val->ToUint32(&length);
> > On 2014/12/15 14:11:00, caitp wrote:
> > > On 2014/12/15 07:23:51, Dmitry Lomov (chromium) wrote:
> > > > This is not good. 
> > > > If ToUInt32 returns false, `length` is uninitialized, but you proceed as
> > > normal.
> > > > No idea what the code below would do.
> > > > It is ok to not support large lengths, but fail gracefully, do not do
> random
> > > > things. 
> > > 
> > > I don't think it can return false here, because `val` is (I think)
> guaranteed
> > to
> > > be a number here. In either case, `length` is initialized to 0 before any
of
> > > this happens, so if it does return false and doesn't initialize `length`,
> it's
> > a
> > > zero value.
> > No, ToString calls user code, so anything can happen.
> > Good point about length 0 though.
> 
> Okay, I'll add some test cases where toString / valueOf return some primitive
> non-numbery things --- other than that it looks ok?

Yes,  +extra tests for toString and valueOf that throws

[caitp (gmail), 2014-12-15 18:15:29]

https://codereview.chromium.org/799853003/diff/20001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-concat.js (right):

https://codereview.chromium.org/799853003/diff/20001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-concat.js:80: "length": {valueOf: null, toString:
null},
On 2014/12/15 07:23:51, Dmitry Lomov (chromium) wrote:
> Add a test where `toString` and `valueOf` throw an exception and validate that
> the exception you get out of `concat` is exact same exception they throw.

Done

[Dmitry Lomov (no reviews), 2014-12-15 18:40:08]

lgtm

[caitp (gmail), 2014-12-15 18:41:26]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2014-12-15 18:41:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799853003/60001

[commit-bot: I haz the power, 2014-12-15 19:08:37]

Committed patchset #4 (id:60001)

[caitp (gmail), 2014-12-16 02:51:31]

On 2014/12/15 19:08:37, I haz the power (commit-bot) wrote:
> Committed patchset #4 (id:60001)

I wrote some followup tests for properties in the prototype chain, and they all
seem to be passing with no extra work --- I think it would be a good idea to add
the extra tests though.

[Dmitry Lomov (no reviews), 2014-12-16 06:59:29]

On 2014/12/16 02:51:31, caitp wrote:
> On 2014/12/15 19:08:37, I haz the power (commit-bot) wrote:
> > Committed patchset #4 (id:60001)
> 
> I wrote some followup tests for properties in the prototype chain, and they
all
> seem to be passing with no extra work --- I think it would be a good idea to
add
> the extra tests though.

Yes, I see, I convinced myself that it all must be working, but please add extra
tests.
They should include:
- arrays and array-likes with holes
- getters and data properties on both object and its prototype.

[commit-bot: I haz the power, 2014-12-17 10:13:44]

Patchset 4 (id:??) landed as
https://crrev.com/aed5d734e66093c54caedf957895764a20db82b1
Cr-Commit-Position: refs/heads/master@{#25828}