[string] Widen StringIndexOf fast path

[string] Widen StringIndexOf fast path

The StringIndexOf fast path used to be very narrow, only allowing
one-byte single-char search strings (and a one-byte subject string).

This changes the CSA fast path to call into our internal SearchString C++
function instead (after attempting to unpack both Strings), and can handle
strings of arbitrary length and encoding. The only remaining runtime call is
when either string needs to be flattened.

BUG=

Review-Url: https://codereview.chromium.org/2814373002
Cr-Commit-Position: refs/heads/master@{#44718}
Committed: https://chromium.googlesource.com/v8/v8/+/4cb011885b95d1f0edcfda5a80906bd9fb2376a3

[jgruber, 2017-04-13 13:11:34]

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

[commit-bot: I haz the power, 2017-04-13 13:11:43]

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-04-13 13:44:48]

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

[commit-bot: I haz the power, 2017-04-13 13:44:48]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-04-18 12:39:41]

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

[commit-bot: I haz the power, 2017-04-18 12:39:47]

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

[jgruber, 2017-04-18 12:41:28]

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

[commit-bot: I haz the power, 2017-04-18 12:41:39]

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-04-18 12:46:08]

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

[commit-bot: I haz the power, 2017-04-18 12:46:09]

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

[jgruber, 2017-04-18 12:52:36]

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

[commit-bot: I haz the power, 2017-04-18 12:52:37]

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

[jgruber, 2017-04-18 13:00:02]

jgruber@chromium.org changed reviewers:
+ cbruni@chromium.org

[jgruber, 2017-04-18 13:00:03]

As discussed offline, dispatch logic could be moved to C++ (with the caveat that
bailouts would need to go back to CSA before calling into runtime through
CEntryStub).

WDYT about landing this as-is for now and moving dispatch to C++ in the future?

[commit-bot: I haz the power, 2017-04-18 13:23:43]

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

[commit-bot: I haz the power, 2017-04-18 13:23:44]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-04-18 14:12:36]

LGTM with nits.

And +1 on future CL to migrate the majority of this to C++

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
File src/builtins/builtins-string-gen.cc (right):

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:108:
ExternalConstant(ExternalReference::isolate_address(isolate()));
I presume this is faster than just doing it in C++?
Otherwise I'd do it there, less noise.

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:807: Bind(&one_one);
BIND(&one_one);

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:848: Bind(&one_two);
BIND(...);

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:861: Bind(&two_one);
BIND(...);

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:874: Bind(&two_two);
BIND(...);

https://codereview.chromium.org/2814373002/diff/60001/src/string-search.h
File src/string-search.h (right):

https://codereview.chromium.org/2814373002/diff/60001/src/string-search.h#new...
src/string-search.h:572: int pattern_length, int start_index) {
DisallowHeapAllocation no_gc();

[jgruber, 2017-04-19 06:59:05]

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
File src/builtins/builtins-string-gen.cc (right):

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:108:
ExternalConstant(ExternalReference::isolate_address(isolate()));
On 2017/04/18 14:12:36, Camillo Bruni wrote:
> I presume this is faster than just doing it in C++?
> Otherwise I'd do it there, less noise.

I'm not sure how we would fetch the current isolate from C++? AFAIK it works in
the builtin because we pass the isolate itself into isolate_addr(isolate).

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:807: Bind(&one_one);
On 2017/04/18 14:12:36, Camillo Bruni wrote:
> BIND(&one_one);

Done.

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:848: Bind(&one_two);
On 2017/04/18 14:12:36, Camillo Bruni wrote:
> BIND(...);

Done.

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:861: Bind(&two_one);
On 2017/04/18 14:12:36, Camillo Bruni wrote:
> BIND(...);

Done.

https://codereview.chromium.org/2814373002/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:874: Bind(&two_two);
On 2017/04/18 14:12:36, Camillo Bruni wrote:
> BIND(...);

Done.

https://codereview.chromium.org/2814373002/diff/60001/src/string-search.h
File src/string-search.h (right):

https://codereview.chromium.org/2814373002/diff/60001/src/string-search.h#new...
src/string-search.h:572: int pattern_length, int start_index) {
On 2017/04/18 14:12:36, Camillo Bruni wrote:
> DisallowHeapAllocation no_gc();

Good idea, done.

[jgruber, 2017-04-19 07:00:04]

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

[commit-bot: I haz the power, 2017-04-19 07:00:13]

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-04-19 07:27:42]

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

[commit-bot: I haz the power, 2017-04-19 07:27:43]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-04-19 07:51:59]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2017-04-19 07:51:59]

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

[commit-bot: I haz the power, 2017-04-19 07:52:08]

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-04-19 07:54:57]

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

[commit-bot: I haz the power, 2017-04-19 07:54:58]

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

[jgruber, 2017-04-19 08:00:09]

jgruber@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[jgruber, 2017-04-19 08:00:10]

+mstarzinger for compiler/, PTAL

[Michael Starzinger, 2017-04-19 10:42:53]

LGTM on "compiler".

[jgruber, 2017-04-19 10:43:59]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2017-04-19 10:44:09]

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-04-19 10:47:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492598639139290,
"parent_rev": "06d2e8128ad93a8ef080e57a10cce266f648e97e", "commit_rev":
"4cb011885b95d1f0edcfda5a80906bd9fb2376a3"}

[commit-bot: I haz the power, 2017-04-19 10:47:09]

Description was changed from

==========
[string] Widen StringIndexOf fast path

The StringIndexOf fast path used to be very narrow, only allowing
one-byte single-char search strings (and a one-byte subject string).

This changes the CSA fast path to call into our internal SearchString C++
function instead (after attempting to unpack both Strings), and can handle
strings of arbitrary length and encoding. The only remaining runtime call is
when either string needs to be flattened.

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

to

==========
[string] Widen StringIndexOf fast path

The StringIndexOf fast path used to be very narrow, only allowing
one-byte single-char search strings (and a one-byte subject string).

This changes the CSA fast path to call into our internal SearchString C++
function instead (after attempting to unpack both Strings), and can handle
strings of arbitrary length and encoding. The only remaining runtime call is
when either string needs to be flattened.

BUG=

Review-Url: https://codereview.chromium.org/2814373002
Cr-Commit-Position: refs/heads/master@{#44718}
Committed:
https://chromium.googlesource.com/v8/v8/+/4cb011885b95d1f0edcfda5a80906bd9fb2...
==========

[commit-bot: I haz the power, 2017-04-19 10:47:10]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/4cb011885b95d1f0edcfda5a80906bd9fb2...