Factor out core of Array.forEach and .every, for use in TypedArrays

Factor out core of Array.forEach and .every, for use in TypedArrays

The idea is to make this the model for future TypedArray methods.
A possible downside could be lower array method performance
if everything gets polymorhpic (but if enough inlining happens, it
should still be fast), but on the upside, this change means that
the TypedArray methods won't create as much code size bloat.

BUG=v8:3578
LOG=Y
R=adamk@chromium.org
CC=arv@chromium.org, caitpotter88@gmail.com

Committed: https://crrev.com/1ebbaaa0362ffc295fc4121bead8bb84a3cf5ec6
Cr-Commit-Position: refs/heads/master@{#28351}

[arv (Not doing code reviews), 2015-05-11 17:45:00]

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

[arv (Not doing code reviews), 2015-05-11 17:45:01]

I like.

We should either use a macro or %SetInlineBuiltinFlag.

https://codereview.chromium.org/1133503003/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/1133503003/diff/1/src/array.js#newcode1185
src/array.js:1185: {
{ before newline

https://codereview.chromium.org/1133503003/diff/1/src/array.js#newcode1197
src/array.js:1197: if (!check_has_index || HAS_INDEX(array, i, is_array)) {
This might be noticable.

Another option would be to use a macro.

https://codereview.chromium.org/1133503003/diff/1/src/array.js#newcode1205
src/array.js:1205: }
Would %SetInlineBuiltinFlag work?

[caitp (gmail), 2015-05-11 17:47:24]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-05-11 17:47:25]

https://codereview.chromium.org/1133503003/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/1133503003/diff/1/src/array.js#newcode1197
src/array.js:1197: if (!check_has_index || HAS_INDEX(array, i, is_array)) {
On 2015/05/11 17:45:01, arv wrote:
> This might be noticable.
> 
> Another option would be to use a macro.

Can js2c resolve macros across different files? If we can do that, that seems
like a good way to go

[adamk, 2015-05-11 19:54:28]

https://codereview.chromium.org/1133503003/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/1133503003/diff/1/src/array.js#newcode1197
src/array.js:1197: if (!check_has_index || HAS_INDEX(array, i, is_array)) {
I think we can leave out check_has_index for now, and optimize the "in" operator
or the HAS_INDEX macro (or both) if we find this showing up in benchmarks.

[dehrenberg, 2015-05-11 22:28:12]

On 2015/05/11 19:54:28, adamk wrote:
> https://codereview.chromium.org/1133503003/diff/1/src/array.js
> File src/array.js (right):
> 
> https://codereview.chromium.org/1133503003/diff/1/src/array.js#newcode1197
> src/array.js:1197: if (!check_has_index || HAS_INDEX(array, i, is_array)) {
> I think we can leave out check_has_index for now, and optimize the "in"
operator
> or the HAS_INDEX macro (or both) if we find this showing up in benchmarks.

Good idea, did that in v2.

[adamk, 2015-05-11 22:39:24]

Thanks for the simplification. I do suspect that this changes behavior of the
neutered-during-forEach case, but the existing test doesn't run long enough to
notice.

Specifically:

https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/mjsunit/ha...

if you change the neutering to happen the first time through, the function won't
be called for index 1, since the HasProperty check should fail. Can you update
the test to exercise this change in behavior (should be as simple as making the
test be "if (count == 0)")?

https://codereview.chromium.org/1133503003/diff/20001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/1133503003/diff/20001/src/array.js#newcode1185
src/array.js:1185: {
I'll re-iterate arv's style nit, please put this open brace on the previous
line.

https://codereview.chromium.org/1133503003/diff/20001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/1133503003/diff/20001/src/harmony-typedarray....
src/harmony-typedarray.js:33: function TypedArrayEvery(f, receiver /* thisArg
*/) {  // length == 1
You can remove the "/* thisArg */" and "// length == 1" comments here, since you
now explicitly set the length below.

https://codereview.chromium.org/1133503003/diff/20001/src/harmony-typedarray....
src/harmony-typedarray.js:43: function TypedArrayForEach(f, receiver /* thisArg
*/) {  // length == 1
Ditto, no need for these comments anymore.

[dehrenberg, 2015-05-11 23:53:11]

On 2015/05/11 22:39:24, adamk wrote:
> Thanks for the simplification. I do suspect that this changes behavior of the
> neutered-during-forEach case, but the existing test doesn't run long enough to
> notice.
> 
> Specifically:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/mjsunit/ha...
> 
> if you change the neutering to happen the first time through, the function
won't
> be called for index 1, since the HasProperty check should fail. Can you update
> the test to exercise this change in behavior (should be as simple as making
the
> test be "if (count == 0)")?

Oh, good point. I "fixed" the test, though now to pass the test you have to
violate the spec :). What do you think?
> 
> https://codereview.chromium.org/1133503003/diff/20001/src/array.js
> File src/array.js (right):
> 
> https://codereview.chromium.org/1133503003/diff/20001/src/array.js#newcode1185
> src/array.js:1185: {
> I'll re-iterate arv's style nit, please put this open brace on the previous
> line.
Done
> 
>
https://codereview.chromium.org/1133503003/diff/20001/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/1133503003/diff/20001/src/harmony-typedarray....
> src/harmony-typedarray.js:33: function TypedArrayEvery(f, receiver /* thisArg
> */) {  // length == 1
> You can remove the "/* thisArg */" and "// length == 1" comments here, since
you
> now explicitly set the length below.
> 
>
https://codereview.chromium.org/1133503003/diff/20001/src/harmony-typedarray....
> src/harmony-typedarray.js:43: function TypedArrayForEach(f, receiver /*
thisArg
> */) {  // length == 1
> Ditto, no need for these comments anymore.

Done

[adamk, 2015-05-12 00:00:38]

Please make the same test alteration in the test for TypedArrayEvery. Once
that's done, lgtm.

[dehrenberg, 2015-05-12 00:11:28]

The CQ bit was checked by dehrenberg@chromium.org

[dehrenberg, 2015-05-12 00:11:28]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1133503003/#ps60001
(title: "extend every test")

[commit-bot: I haz the power, 2015-05-12 00:11:43]

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

[commit-bot: I haz the power, 2015-05-12 00:12:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 00:12:58]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/287)

[dehrenberg, 2015-05-12 00:35:15]

The CQ bit was checked by dehrenberg@chromium.org

[dehrenberg, 2015-05-12 00:35:16]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1133503003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-12 00:35:41]

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

[commit-bot: I haz the power, 2015-05-12 01:32:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-12 01:32:46]

Patchset 5 (id:??) landed as
https://crrev.com/1ebbaaa0362ffc295fc4121bead8bb84a3cf5ec6
Cr-Commit-Position: refs/heads/master@{#28351}