[builtins] TurboFan version of Array.prototype.forEach including fast path for FAST_ELEMENTS

[builtins] TurboFan version of Array.prototype.forEach including fast path for FAST_ELEMENTS

BUG=v8:5269, v8:1956
LOG=N
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2663033003
Cr-Commit-Position: refs/heads/master@{#42926}
Committed: https://chromium.googlesource.com/v8/v8/+/5205b9db0659efc03f612e6ffc525b922629d67c

[danno, 2017-01-31 14:14:49]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 14:14:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 14:19:49]

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

[commit-bot: I haz the power, 2017-01-31 14:19:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[danno, 2017-01-31 14:37:11]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 14:37:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 14:42:08]

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

[commit-bot: I haz the power, 2017-01-31 14:42:09]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[danno, 2017-01-31 14:45:51]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 14:45:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 15:02:09]

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

[commit-bot: I haz the power, 2017-01-31 15:02:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[danno, 2017-01-31 21:34:39]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 21:34:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 21:52:57]

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

[commit-bot: I haz the power, 2017-01-31 21:52:58]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[danno, 2017-01-31 22:45:23]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 22:45:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 23:13:30]

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

[commit-bot: I haz the power, 2017-01-31 23:13:31]

Dry run: This issue passed the CQ dry run.

[danno, 2017-01-31 23:14:19]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 23:14:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-02-01 07:16:36]

Description was changed from

==========
[builtins] TurboFan version of Array.prototype.forEach including fast path for
FAST_ELEMENTS

BUG=v8:5269
LOG=N
R=bmeurer@chromium.org
==========

to

==========
[builtins] TurboFan version of Array.prototype.forEach including fast path for
FAST_ELEMENTS

BUG=v8:5269, v8:1956
LOG=N
R=bmeurer@chromium.org
==========

[danno, 2017-02-02 00:15:24]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 00:15:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-02-02 00:16:17]

danno@chromium.org changed reviewers:
+ yangguo@chromium.org

[danno, 2017-02-02 00:16:18]

ptal. Yang, would you mind looking at the boostrapper/builtins registration
logic to allow activating the new flag without having to rebuild? Thanks!

[commit-bot: I haz the power, 2017-02-02 00:34:59]

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

[commit-bot: I haz the power, 2017-02-02 00:35:00]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Benedikt Meurer, 2017-02-02 05:22:38]

Looking good! First round of comments.

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:508: }
You need to handle holey arrays here. I.e. upon reading the hole/hole NaN you
must verify that the array prototype chain is still intact, and if not go to the
slow path, otherwise skip the hole.

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:540: Variable callbackfn(this,
MachineRepresentation::kTagged);
Wouldn't it be easier to just use the arguments adaptor mechanism here? Then
you'll always have callbackFn and thisArg parameters, and if the user supplied
too few or too many, the arguments adaptor will just fill up with undefined.

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:548: // TODO(danno): Seriously? Do we really need
to throw the exact error message
I think it's fine to just let ToObject fail and rebaseline the layout tests; so
don't do this additional check.

[Yang, 2017-02-02 10:04:59]

https://codereview.chromium.org/2663033003/diff/230001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/2663033003/diff/230001/src/js/array.js#newcod...
src/js/array.js:1580: specialExperimentalArrayFunctions =
%SpecialExperimentalArrayFunctions();
Can we have all of this in C++, in the bootstrapper?
See InitializeExperimentalGlobal. For an example, see
InitializeGlobal_harmony_sharedarraybuffer. That code is run after deserializing
the snapshot and can act on flags at runtime.

You would simply get the Array prototype from the global object, and install a
different built-in.

And please make sure the initial_array_prototype on the context is up-to-date
after you overwrite forEach.

[danno, 2017-02-02 15:22:49]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 15:22:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-02-02 15:23:41]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 15:23:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 15:25:15]

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

[commit-bot: I haz the power, 2017-02-02 15:25:16]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20348)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[danno, 2017-02-02 15:26:46]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 15:26:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-02-02 15:49:54]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 15:50:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 16:19:25]

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

[commit-bot: I haz the power, 2017-02-02 16:19:27]

Dry run: This issue passed the CQ dry run.

[danno, 2017-02-02 16:59:58]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 17:00:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-02-02 17:01:01]

Feedback addressed, please take another look.

[danno, 2017-02-02 17:15:48]

Now with comments addressed.

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:508: }
On 2017/02/02 05:22:38, Benedikt Meurer wrote:
> You need to handle holey arrays here. I.e. upon reading the hole/hole NaN you
> must verify that the array prototype chain is still intact, and if not go to
the
> slow path, otherwise skip the hole.

Done.

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:540: Variable callbackfn(this,
MachineRepresentation::kTagged);
On 2017/02/02 05:22:38, Benedikt Meurer wrote:
> Wouldn't it be easier to just use the arguments adaptor mechanism here? Then
> you'll always have callbackFn and thisArg parameters, and if the user supplied
> too few or too many, the arguments adaptor will just fill up with undefined.

Done.

https://codereview.chromium.org/2663033003/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:548: // TODO(danno): Seriously? Do we really need
to throw the exact error message
On 2017/02/02 05:22:38, Benedikt Meurer wrote:
> I think it's fine to just let ToObject fail and rebaseline the layout tests;
so
> don't do this additional check.

For consistency with the other Array builtins, I'm going to leave this as-is for
now. We should go back and then change all of them to be simpler and consistent
in a separate CL.

https://codereview.chromium.org/2663033003/diff/230001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/2663033003/diff/230001/src/js/array.js#newcod...
src/js/array.js:1580: specialExperimentalArrayFunctions =
%SpecialExperimentalArrayFunctions();
On 2017/02/02 10:04:58, Yang wrote:
> Can we have all of this in C++, in the bootstrapper?
> See InitializeExperimentalGlobal. For an example, see
> InitializeGlobal_harmony_sharedarraybuffer. That code is run after
deserializing
> the snapshot and can act on flags at runtime.
> 
> You would simply get the Array prototype from the global object, and install a
> different built-in.
> 
> And please make sure the initial_array_prototype on the context is up-to-date
> after you overwrite forEach.

Done.

[commit-bot: I haz the power, 2017-02-02 17:20:29]

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

[commit-bot: I haz the power, 2017-02-02 17:20:30]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2017-02-02 17:54:05]

Thanks. LGTM from my side.

[Yang, 2017-02-03 10:20:26]

LGTM with comments.

https://codereview.chromium.org/2663033003/diff/320001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2663033003/diff/320001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:3555:
Handle<JSObject>::cast(isolate->natives_utils_object());
Alternatively, you could just get global.Array.prototype.forEach, and won't have
to touch the utils object at all.

https://codereview.chromium.org/2663033003/diff/320001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2663033003/diff/320001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:86:
RUNTIME_FUNCTION(Runtime_SpecialExperimentalArrayFunctions) {
This is no longer necessary, is it?

[danno, 2017-02-03 12:46:55]

Feedback addressed, landing

https://codereview.chromium.org/2663033003/diff/320001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2663033003/diff/320001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:3555:
Handle<JSObject>::cast(isolate->natives_utils_object());
On 2017/02/03 10:20:25, Yang wrote:
> Alternatively, you could just get global.Array.prototype.forEach, and won't
have
> to touch the utils object at all.

Done.

https://codereview.chromium.org/2663033003/diff/320001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2663033003/diff/320001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:86:
RUNTIME_FUNCTION(Runtime_SpecialExperimentalArrayFunctions) {
On 2017/02/03 10:20:25, Yang wrote:
> This is no longer necessary, is it?

Done.

[danno, 2017-02-03 12:47:03]

The CQ bit was checked by danno@chromium.org

[danno, 2017-02-03 12:47:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2663033003/#ps340001
(title: "Review feedback")

[commit-bot: I haz the power, 2017-02-03 12:47:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 13:16:20]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1486126023539390,
"parent_rev": "8c7fc377fd5c03e30cbf767cd22aba59178e0143", "commit_rev":
"5205b9db0659efc03f612e6ffc525b922629d67c"}

[commit-bot: I haz the power, 2017-02-03 13:16:25]

Description was changed from

==========
[builtins] TurboFan version of Array.prototype.forEach including fast path for
FAST_ELEMENTS

BUG=v8:5269, v8:1956
LOG=N
R=bmeurer@chromium.org
==========

to

==========
[builtins] TurboFan version of Array.prototype.forEach including fast path for
FAST_ELEMENTS

BUG=v8:5269, v8:1956
LOG=N
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2663033003
Cr-Commit-Position: refs/heads/master@{#42926}
Committed:
https://chromium.googlesource.com/v8/v8/+/5205b9db0659efc03f612e6ffc525b92262...
==========

[commit-bot: I haz the power, 2017-02-03 13:16:26]

Committed patchset #19 (id:340001) as
https://chromium.googlesource.com/v8/v8/+/5205b9db0659efc03f612e6ffc525b92262...