[builtins] Array.prototype.filter implemented as a TurboFan code stub.

[builtins] Array.prototype.filter implemented as a TurboFan code stub.

BUG=

Review-Url: https://codereview.chromium.org/2680153005
Cr-Commit-Position: refs/heads/master@{#43965}
Committed: https://chromium.googlesource.com/v8/v8/+/1e03479c64ab3dc41182f4d7017b1fb39ceffb27

[mvstanton, 2017-03-18 13:05:53]

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

[commit-bot: I haz the power, 2017-03-18 13:05:56]

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-03-18 13:11:06]

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

[commit-bot: I haz the power, 2017-03-18 13:11:07]

Dry run: Try jobs failed on following builders:
  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...)

[mvstanton, 2017-03-18 18:50:13]

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

[commit-bot: I haz the power, 2017-03-18 18:50:23]

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-03-18 18:53:52]

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

[commit-bot: I haz the power, 2017-03-18 18:53:53]

Dry run: Try jobs failed on following builders:
  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...)

[mvstanton, 2017-03-19 10:06:04]

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

[mvstanton, 2017-03-19 10:06:05]

HI Danno,
I'd like to get this checked into the tree so I don't have to keep rebasing
(:p)....

I've added a flag so that I can keep refactoring and doing more work this week
(for example, removing the CreateDataProperty calls). But the performance is
only ~10% down from what we have now in the js-perf benchmarks I've added, so we
are getting close. (and we compile less).

Thanks for the look!
--Michael

[mvstanton, 2017-03-19 10:06:18]

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

[commit-bot: I haz the power, 2017-03-19 10:06:21]

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-03-19 10:11:11]

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

[commit-bot: I haz the power, 2017-03-19 10:11:11]

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-03-20 08:06:26]

Totally on the right track. I wonder if we can't simplify a bit, let's discuss
when I get in.

https://codereview.chromium.org/2680153005/diff/100001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2680153005/diff/100001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:3709: if (FLAG_csa_builtin_filter) {
Instead of introducing a new flag, how about you just remove the implication
from --turbo on FLAG_enable_fast_array_builtins, and then use that flag to
check/bail out here if it's not set, and all new "experimental" builtins would
be installed after that check.

https://codereview.chromium.org/2680153005/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-array-gen.cc (right):

https://codereview.chromium.org/2680153005/diff/100001/src/builtins/builtins-...
src/builtins/builtins-array-gen.cc:195: GetProperty(context, a,
isolate()->factory()->length_string());
Careful, this call is observable. I don't think it's safe to access the property
like this. We'll have to discuss how it's safe to pass this through, I think it
may indeed need to be a parameter to the the Continuation builder.

https://codereview.chromium.org/2680153005/diff/100001/src/builtins/builtins-...
src/builtins/builtins-array-gen.cc:295: processor(context, value, a,
callback_result);
Instead of adding new parameters, I think it's cleaner to have accessors in the
Assembler.

[mvstanton, 2017-03-20 09:56:51]

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

[commit-bot: I haz the power, 2017-03-20 09:56: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-03-20 10:01:28]

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

[commit-bot: I haz the power, 2017-03-20 10:01:29]

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...)

[mvstanton, 2017-03-20 16:08:24]

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

[commit-bot: I haz the power, 2017-03-20 16:08:27]

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

[mvstanton, 2017-03-20 16:09:17]

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

[commit-bot: I haz the power, 2017-03-20 16:09:21]

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

[mvstanton, 2017-03-20 16:10:53]

Hi Danno,
Thanks for the review.

For the first comment (about flags), this CL is in the works
(https://codereview.chromium.org/2761783002/).

For the second (observable call), I'll address it straightaway after landing
this.

For the third, I'll wait for your CL and assist you in rebase/etc to eliminate
the parameter.

Thanks!
--Michael

https://codereview.chromium.org/2680153005/diff/100001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2680153005/diff/100001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:3709: if (FLAG_csa_builtin_filter) {
On 2017/03/20 08:06:25, danno wrote:
> Instead of introducing a new flag, how about you just remove the implication
> from --turbo on FLAG_enable_fast_array_builtins, and then use that flag to
> check/bail out here if it's not set, and all new "experimental" builtins would
> be installed after that check.

I like the idea, but right now the filter implementation still needs work (for
example the CreateDataProperty call we want to eliminate). Being able to turn it
on and off (and it'll be off by default in this first CL) lets me run the
microbenchmarks at the command prompt and see the difference. I don't want to
permute the environment by turning off our other CSAs...

The flag is temporary and should disappear within a couple weeks...would that be
okay?

https://codereview.chromium.org/2680153005/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-array-gen.cc (right):

https://codereview.chromium.org/2680153005/diff/100001/src/builtins/builtins-...
src/builtins/builtins-array-gen.cc:195: GetProperty(context, a,
isolate()->factory()->length_string());
On 2017/03/20 08:06:25, danno wrote:
> Careful, this call is observable. I don't think it's safe to access the
property
> like this. We'll have to discuss how it's safe to pass this through, I think
it
> may indeed need to be a parameter to the the Continuation builder.

Good catch! Actually, test262 didn't even see this (I had a failure there that
led me to introduce the GetProperty() call here).

I'd like to check in with this issue and with a follow on CL fix it by passing a
parameter to the continuation builder. Since the implementation isn't turned on
yet, that should be reasonable.

https://codereview.chromium.org/2680153005/diff/100001/src/builtins/builtins-...
src/builtins/builtins-array-gen.cc:295: processor(context, value, a,
callback_result);
On 2017/03/20 08:06:25, danno wrote:
> Instead of adding new parameters, I think it's cleaner to have accessors in
the
> Assembler.

Yep, as per your upcoming CL. I'll leave this one alone and then religiously
follow the model you have in that one.

[commit-bot: I haz the power, 2017-03-20 16:17:12]

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

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

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-03-20 16:58:38]

https://codereview.chromium.org/2680153005/diff/160001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2680153005/diff/160001/src/flag-definitions.h...
src/flag-definitions.h:277: "Experimental versions of array builtins")
Consider moving this to the section in this file "// builtins.cc"

https://codereview.chromium.org/2680153005/diff/160001/src/interface-descript...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/2680153005/diff/160001/src/interface-descript...
src/interface-descriptors.h:730: class ArrayFilterDescriptor : public
BuiltinDescriptor {
I don't think you need this at all anymore....

https://codereview.chromium.org/2680153005/diff/160001/test/js-perf-test/Arra...
File test/js-perf-test/Array/filter.js (right):

https://codereview.chromium.org/2680153005/diff/160001/test/js-perf-test/Arra...
test/js-perf-test/Array/filter.js:1: // Copyright 2014 the V8 project authors.
All rights reserved.
nit: Copyright date

[mvstanton, 2017-03-20 22:08:04]

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

[commit-bot: I haz the power, 2017-03-20 22:08:10]

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

[mvstanton, 2017-03-20 22:08:31]

Thanks for the look!
Comments addressed...

--Michael

https://codereview.chromium.org/2680153005/diff/160001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2680153005/diff/160001/src/flag-definitions.h...
src/flag-definitions.h:277: "Experimental versions of array builtins")
On 2017/03/20 16:58:38, danno wrote:
> Consider moving this to the section in this file "// builtins.cc"

Done.

https://codereview.chromium.org/2680153005/diff/160001/src/interface-descript...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/2680153005/diff/160001/src/interface-descript...
src/interface-descriptors.h:730: class ArrayFilterDescriptor : public
BuiltinDescriptor {
On 2017/03/20 16:58:38, danno wrote:
> I don't think you need this at all anymore....

Done.

https://codereview.chromium.org/2680153005/diff/160001/test/js-perf-test/Arra...
File test/js-perf-test/Array/filter.js (right):

https://codereview.chromium.org/2680153005/diff/160001/test/js-perf-test/Arra...
test/js-perf-test/Array/filter.js:1: // Copyright 2014 the V8 project authors.
All rights reserved.
On 2017/03/20 16:58:38, danno wrote:
> nit: Copyright date

Done.

[commit-bot: I haz the power, 2017-03-20 22:34:29]

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

[commit-bot: I haz the power, 2017-03-20 22:34:30]

Dry run: This issue passed the CQ dry run.

[danno, 2017-03-21 08:53:23]

lgtm. as discussed, there is cleanup needed here that will happen automatically
when I merge/land my follow-on CL.

[mvstanton, 2017-03-21 08:54:07]

The CQ bit was checked by mvstanton@chromium.org

[commit-bot: I haz the power, 2017-03-21 08:54:17]

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-03-21 08:56:57]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490086447809820,
"parent_rev": "ed93e7c27b2c3f2ddd995fdc3a40b35341232e9f", "commit_rev":
"1e03479c64ab3dc41182f4d7017b1fb39ceffb27"}

[commit-bot: I haz the power, 2017-03-21 08:57:02]

Description was changed from

==========
[builtins] Array.prototype.filter implemented as a TurboFan code stub.

BUG=
==========

to

==========
[builtins] Array.prototype.filter implemented as a TurboFan code stub.

BUG=

Review-Url: https://codereview.chromium.org/2680153005
Cr-Commit-Position: refs/heads/master@{#43965}
Committed:
https://chromium.googlesource.com/v8/v8/+/1e03479c64ab3dc41182f4d7017b1fb39ce...
==========

[commit-bot: I haz the power, 2017-03-21 08:57:03]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/v8/v8/+/1e03479c64ab3dc41182f4d7017b1fb39ce...

[Michael Achenbach, 2017-03-22 08:52:57]

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

[Michael Achenbach, 2017-03-22 08:52:58]

https://codereview.chromium.org/2680153005/diff/180001/test/js-perf-test/JSTe...
File test/js-perf-test/JSTests.json (right):

https://codereview.chromium.org/2680153005/diff/180001/test/js-perf-test/JSTe...
test/js-perf-test/JSTests.json:332: "name": "Arrays",
The folder was called Array. This fails now on bots. Please fix asap as it
prevents uploads for the other tests too.

[agrieve, 2017-04-26 00:00:12]

On 2017/03/22 08:52:58, Michael Achenbach wrote:
>
https://codereview.chromium.org/2680153005/diff/180001/test/js-perf-test/JSTe...
> File test/js-perf-test/JSTests.json (right):
> 
>
https://codereview.chromium.org/2680153005/diff/180001/test/js-perf-test/JSTe...
> test/js-perf-test/JSTests.json:332: "name": "Arrays",
> The folder was called Array. This fails now on bots. Please fix asap as it
> prevents uploads for the other tests too.

Could someone that reviewed this change please comment on its size impact here:
https://bugs.chromium.org/p/chromium/issues/detail?id=704181