Fix Array.prototype.push and Array.prototype.unshift for read-only length.

Fix Array.prototype.push and Array.prototype.unshift for read-only length.

BUG=
R=mstarzinger@chromium.org, mvstanton@chromium.org

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

[ulan, 2014-05-09 08:44:58]

PTAL

[ulan, 2014-05-09 09:37:12]

On 2014/05/09 08:44:58, ulan wrote:
> PTAL

Found a flaw, this doesn't work with optimized push/unshift. I will upload a new
patch set with a fix later.

[ulan, 2014-05-09 12:26:56]

I uploaded a new patch set with a hydrogen fix. PTAL.

[mvstanton, 2014-05-09 12:56:09]

Couple of comments, if addressed then lgtm.

https://codereview.chromium.org/279773002/diff/100001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/279773002/diff/100001/src/builtins.cc#newcode387
src/builtins.cc:387: if (to_add > 0 && JSArray::ChangeOfReadOnlyLength(array,
len + to_add)) {
The name of this function is odd, how about JSArray::CanChangeLength()?

Also, it definitely seems like we need it, but I wonder how costly the lookup of
the length property attributes is: okay from a performance view?

[Michael Starzinger, 2014-05-14 08:30:47]

Looking good.

https://codereview.chromium.org/279773002/diff/120001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/279773002/diff/120001/src/builtins.cc#newcode390
src/builtins.cc:390: JSArray::ReadOnlyLengthError(array));
As discussed offline: Instead of handling this corner-case here in the optimized
version, could we instead call out to the JS builtin reference implementation
instead? This would make the optimized path simpler.

https://codereview.chromium.org/279773002/diff/120001/src/builtins.cc#newcode594
src/builtins.cc:594: JSArray::ReadOnlyLengthError(array));
Likewise.

[ulan, 2014-05-15 11:15:53]

Thank you, I uploaded new patch.

Note that I changed KeyedStoreIC in ic.cc: there was a bug where we would set
the ic target even in case when runtime returned exception.

https://codereview.chromium.org/279773002/diff/120001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/279773002/diff/120001/src/builtins.cc#newcode390
src/builtins.cc:390: JSArray::ReadOnlyLengthError(array));
On 2014/05/14 08:30:48, Michael Starzinger wrote:
> As discussed offline: Instead of handling this corner-case here in the
optimized
> version, could we instead call out to the JS builtin reference implementation
> instead? This would make the optimized path simpler.

Replaced RETURN_FAILURE_ON_EXCEPTION(..) with call to JS builtin. I thought
about simplifying the condition as well (only checking for read-only length),
but left it unchanged because it allows to avoid length lookup when the index is
within bounds.

[Michael Starzinger, 2014-05-21 12:34:36]

LGTM. Just a couple of nits and suggestions left.

https://codereview.chromium.org/279773002/diff/160001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/279773002/diff/160001/src/array.js#newcode606
src/array.js:606: if (len > 0) {
As discussed offline: IMHO SimpleMove shouldn't have any side-effects if len ==
0 here. So this check should be obsolete.

https://codereview.chromium.org/279773002/diff/160001/src/array.js#newcode634
src/array.js:634: if (IS_ARRAY(array) && !is_sealed) {
Likewise, for len == 0 should do the right thing here, so can we restructure
this as follows instead?

if (IS_ARRAY(array) && !is_sealed && len > 0) {
  SmartMove(array, 0, 0, len, num_arguments);
} else {
  SimpleMove(array, 0, 0, len, num_arguments);
}

https://codereview.chromium.org/279773002/diff/160001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/279773002/diff/160001/src/builtins.cc#newcode592
src/builtins.cc:592: 
nit: Only one empty new-line is fine.

https://codereview.chromium.org/279773002/diff/160001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/279773002/diff/160001/src/ic.cc#newcode1802
src/ic.cc:1802: result,
nit: We should be able to assign to store_handle here, no need for another local
variable. Makes the return statement below simpler.

https://codereview.chromium.org/279773002/diff/160001/src/ic.cc#newcode1804
src/ic.cc:1804: isolate(), object, key, value,  NONE, strict_mode()),
nit: Only one white-space in front of NONE here.

[ulan, 2014-05-21 14:52:21]

Thank you for review. Landing.

https://codereview.chromium.org/279773002/diff/160001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/279773002/diff/160001/src/array.js#newcode606
src/array.js:606: if (len > 0) {
On 2014/05/21 12:34:36, Michael Starzinger wrote:
> As discussed offline: IMHO SimpleMove shouldn't have any side-effects if len
==
> 0 here. So this check should be obsolete.

Done.

https://codereview.chromium.org/279773002/diff/160001/src/array.js#newcode634
src/array.js:634: if (IS_ARRAY(array) && !is_sealed) {
On 2014/05/21 12:34:36, Michael Starzinger wrote:
> Likewise, for len == 0 should do the right thing here, so can we restructure
> this as follows instead?
> 
> if (IS_ARRAY(array) && !is_sealed && len > 0) {
>   SmartMove(array, 0, 0, len, num_arguments);
> } else {
>   SimpleMove(array, 0, 0, len, num_arguments);
> }

Done.

https://codereview.chromium.org/279773002/diff/160001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/279773002/diff/160001/src/builtins.cc#newcode592
src/builtins.cc:592: 
On 2014/05/21 12:34:36, Michael Starzinger wrote:
> nit: Only one empty new-line is fine.

Done.

https://codereview.chromium.org/279773002/diff/160001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/279773002/diff/160001/src/ic.cc#newcode1802
src/ic.cc:1802: result,
On 2014/05/21 12:34:36, Michael Starzinger wrote:
> nit: We should be able to assign to store_handle here, no need for another
local
> variable. Makes the return statement below simpler.

Done.

https://codereview.chromium.org/279773002/diff/160001/src/ic.cc#newcode1804
src/ic.cc:1804: isolate(), object, key, value,  NONE, strict_mode()),
On 2014/05/21 12:34:36, Michael Starzinger wrote:
> nit: Only one white-space in front of NONE here.

Done.

[ulan, 2014-05-22 08:10:06]

Committed patchset #9 manually as r21423 (presubmit successful).