[string] Move String.p.toLowerCase to CSA

[string] Move String.p.toLowerCase to CSA

This CL migrates the CPP builtin to CSA with fast paths for strings
that can be unpacked to direct one-byte strings. Short strings are
handled directly in CSA, others need to call into C for conversion.

Microbenchmarks for "abcd".toLowerCase() show speedups of 2.5x.

BUG=v8:6353, v8:6344
Review-Url: https://codereview.chromium.org/2859203002
Cr-Commit-Position: refs/heads/master@{#45141}
Committed: https://chromium.googlesource.com/v8/v8/+/f0e95769dbc3c209f513c68e2aa7c9433c4cd934

[jgruber, 2017-05-04 13:45:28]

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

[commit-bot: I haz the power, 2017-05-04 13:45:38]

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-05-04 13:45:39]

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

[commit-bot: I haz the power, 2017-05-04 13:45:40]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[jgruber, 2017-05-04 13:49:33]

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

[commit-bot: I haz the power, 2017-05-04 13:49:38]

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-05-04 14:20:18]

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

[commit-bot: I haz the power, 2017-05-04 14:20:19]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-05-05 06:27:38]

Description was changed from

==========
[string] Move String.p.toLowerCase to CSA

String.p.toLowerCase shows up fairly high on the ARES-6 Basic benchmark.
This CL migrates the CPP builtin to CSA with fast paths for direct
one-byte strings. Short lower-case strings are handled directly in CSA,
others need to call into C for conversion.

BUG=v8:6353,v8:6344
==========

to

==========
[string] Move String.p.toLowerCase to CSA

This CL migrates the CPP builtin to CSA with fast paths for direct
one-byte strings. Short lower-case strings are handled directly in CSA,
others need to call into C for conversion.

BUG=v8:6353,v8:6344
==========

[jgruber, 2017-05-05 08:46:38]

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

[commit-bot: I haz the power, 2017-05-05 08:46:50]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-05-05 08:54:05]

Description was changed from

==========
[string] Move String.p.toLowerCase to CSA

This CL migrates the CPP builtin to CSA with fast paths for direct
one-byte strings. Short lower-case strings are handled directly in CSA,
others need to call into C for conversion.

BUG=v8:6353,v8:6344
==========

to

==========
[string] Move String.p.toLowerCase to CSA

This CL migrates the CPP builtin to CSA with fast paths for strings
that can be unpacked to direct one-byte strings. Short strings are
handled directly in CSA, others need to call into C for conversion.

Microbenchmarks for "abcd".toLowerCase() show speedups of 2.5x.

BUG=v8:6353,v8:6344
==========

[jgruber, 2017-05-05 09:04:00]

jgruber@chromium.org changed reviewers:
+ bmeurer@chromium.org, cbruni@chromium.org, littledan@chromium.org

[jgruber, 2017-05-05 09:04:01]

Short reviewers guide:

This moves S.p.toLowerCase to CSA. 
* Empty strings and unpackable one-byte strings up to length 24 are handled
directly in CSA: They alloc a new string, and fill it with lower-cased chars by
using the lookup table from intl.cc.
* Longer unpackable one-byte strings go through a direct C call to
ConvertOneByteToLower in intl.cc.
* Others take the slow path through a runtime call to StringToLowerCaseIntl
(this is basically equivalent to the old implementation except for additional
overhead for the builtin call).

We could do something similar for toUpperCase (with some additional
complications for 'ß').

PTAL.

https://codereview.chromium.org/2859203002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/2859203002/diff/40001/src/assembler.h#newcode998
src/assembler.h:998: static ExternalReference
intl_to_latin1_lower_table(Isolate* isolate);
I so wanted to call this intl_to_latin1_lover_table

[jgruber, 2017-05-05 09:07:22]

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

[commit-bot: I haz the power, 2017-05-05 09:07:31]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-05-05 09:26:44]

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

[commit-bot: I haz the power, 2017-05-05 09:26:52]

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-05-05 09:55:40]

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

[commit-bot: I haz the power, 2017-05-05 09:55:40]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-05-05 10:10:03]

https://codereview.chromium.org/2859203002/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:4087: 0, true),
We should probably avoid the explicit arguments adaptor frame for TF builtins.
See https://codereview.chromium.org/2829093004
Sounds like something for a follow-up performance CL

https://codereview.chromium.org/2859203002/diff/60001/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:40:
GotoIfNot(IsOneByteStringInstanceType(to_direct.instance_type()), &runtime);
I'd prefer to check for IsOneByteSequential here, that's the only case you
handle anyway (unles I misread ToDirectStringAssembler and we return the
instance_type of the outer string).

https://codereview.chromium.org/2859203002/diff/60001/src/external-reference-...
File src/external-reference-table.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/external-reference-...
src/external-reference-table.cc:256: "intl_to_latin1_lower_table");
Don't you need the #ifdef V8_INTL_SUPPORT here as well?

https://codereview.chromium.org/2859203002/diff/60001/src/intl.cc
File src/intl.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/intl.cc#newcode211
src/intl.cc:211: DCHECK(src->length() == dst->length());
nit: DCHECK_EQ

https://codereview.chromium.org/2859203002/diff/80001/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/80001/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:48: &call_c);
I think we miss a test case for this corner case.
length > 24, onebyte

[jgruber, 2017-05-05 10:21:58]

https://codereview.chromium.org/2859203002/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:4087: 0, true),
On 2017/05/05 10:10:03, Camillo Bruni wrote:
> We should probably avoid the explicit arguments adaptor frame for TF builtins.
> See https://codereview.chromium.org/2829093004
> Sounds like something for a follow-up performance CL

sgtm. I got failures related to stack ptr / exception handling without argument
adaption. Will take a look in a follow-up.

https://codereview.chromium.org/2859203002/diff/60001/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:40:
GotoIfNot(IsOneByteStringInstanceType(to_direct.instance_type()), &runtime);
On 2017/05/05 10:10:03, Camillo Bruni wrote:
> I'd prefer to check for IsOneByteSequential here, that's the only case you
> handle anyway (unles I misread ToDirectStringAssembler and we return the
> instance_type of the outer string).

If we reach this point the string is guaranteed to be sequential or external,
and we handle both cases in the following code. We can't check for IsOneByteSeq
because it could be external.

https://codereview.chromium.org/2859203002/diff/60001/src/external-reference-...
File src/external-reference-table.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/external-reference-...
src/external-reference-table.cc:256: "intl_to_latin1_lower_table");
On 2017/05/05 10:10:03, Camillo Bruni wrote:
> Don't you need the #ifdef V8_INTL_SUPPORT here as well?

Right, nice catch.

https://codereview.chromium.org/2859203002/diff/60001/src/intl.cc
File src/intl.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/intl.cc#newcode211
src/intl.cc:211: DCHECK(src->length() == dst->length());
On 2017/05/05 10:10:03, Camillo Bruni wrote:
> nit: DCHECK_EQ

I'm kinda torn on DCHECK_EQ vs. DCHECK uses. People aren't consistent about
which one they use, and I actually find the plain DCHECK more readable in many
cases. Anyway, not a big deal so I'll change it here.

[jgruber, 2017-05-05 10:39:05]

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

[jgruber, 2017-05-05 10:39:06]

https://codereview.chromium.org/2859203002/diff/80001/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/80001/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:48: &call_c);
On 2017/05/05 10:10:03, Camillo Bruni wrote:
> I think we miss a test case for this corner case.
> length > 24, onebyte

string-case.js should cover this but it flattened incorrectly. Fixed it in this
patchset.

[Camillo Bruni, 2017-05-05 11:44:26]

LGTM with some minor nits.

https://codereview.chromium.org/2859203002/diff/60001/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:40:
GotoIfNot(IsOneByteStringInstanceType(to_direct.instance_type()), &runtime);
On 2017/05/05 at 10:21:58, jgruber wrote:
> On 2017/05/05 10:10:03, Camillo Bruni wrote:
> > I'd prefer to check for IsOneByteSequential here, that's the only case you
> > handle anyway (unles I misread ToDirectStringAssembler and we return the
> > instance_type of the outer string).
> 
> If we reach this point the string is guaranteed to be sequential or external,
and we handle both cases in the following code. We can't check for IsOneByteSeq
because it could be external.

right, I thought I missed a case. But it would definitely never be a cons
string.

https://codereview.chromium.org/2859203002/diff/60001/src/intl.cc
File src/intl.cc (right):

https://codereview.chromium.org/2859203002/diff/60001/src/intl.cc#newcode211
src/intl.cc:211: DCHECK(src->length() == dst->length());
On 2017/05/05 at 10:21:58, jgruber wrote:
> On 2017/05/05 10:10:03, Camillo Bruni wrote:
> > nit: DCHECK_EQ
> 
> I'm kinda torn on DCHECK_EQ vs. DCHECK uses. People aren't consistent about
which one they use, and I actually find the plain DCHECK more readable in many
cases. Anyway, not a big deal so I'll change it here.

I think this is one of these half-finished refactorings :)

https://codereview.chromium.org/2859203002/diff/40002/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/40002/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:40:
GotoIfNot(IsOneByteStringInstanceType(to_direct.instance_type()), &runtime);
Would you mind adding the following assert (I think the NOT should work :P)?
CSA_ASSERT(WordNot(IsConsStringInstanceType(to_direct.instance_type()));

https://codereview.chromium.org/2859203002/diff/40002/src/intl.cc
File src/intl.cc (right):

https://codereview.chromium.org/2859203002/diff/40002/src/intl.cc#newcode229
src/intl.cc:229: // process the rest.
nit: this comment makes probably more sense before the for-loop.

[jgruber, 2017-05-05 15:26:25]

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

[commit-bot: I haz the power, 2017-05-05 15:26:35]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-05-05 15:26:58]

https://codereview.chromium.org/2859203002/diff/40002/src/builtins/builtins-i...
File src/builtins/builtins-intl-gen.cc (right):

https://codereview.chromium.org/2859203002/diff/40002/src/builtins/builtins-i...
src/builtins/builtins-intl-gen.cc:40:
GotoIfNot(IsOneByteStringInstanceType(to_direct.instance_type()), &runtime);
On 2017/05/05 11:44:26, Camillo Bruni wrote:
> Would you mind adding the following assert (I think the NOT should work :P)?
> CSA_ASSERT(WordNot(IsConsStringInstanceType(to_direct.instance_type()));

Sure, we can just assert that the string is not indirect. Will do.

https://codereview.chromium.org/2859203002/diff/40002/src/intl.cc
File src/intl.cc (right):

https://codereview.chromium.org/2859203002/diff/40002/src/intl.cc#newcode229
src/intl.cc:229: // process the rest.
On 2017/05/05 11:44:26, Camillo Bruni wrote:
> nit: this comment makes probably more sense before the for-loop.

Yep, thought so as well. Done.

[jgruber, 2017-05-05 15:49:34]

The CQ bit was unchecked by jgruber@chromium.org

[jgruber, 2017-05-05 15:49:39]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2017-05-05 15:49:40]

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/2859203002/#ps110001
(title: "Address comments")

[commit-bot: I haz the power, 2017-05-05 15:49:59]

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-05-05 15:59:09]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1493999379592340,
"parent_rev": "1cda1732a7d285683aa7e466fbf392744ca6bd99", "commit_rev":
"f0e95769dbc3c209f513c68e2aa7c9433c4cd934"}

[commit-bot: I haz the power, 2017-05-05 15:59:17]

Description was changed from

==========
[string] Move String.p.toLowerCase to CSA

This CL migrates the CPP builtin to CSA with fast paths for strings
that can be unpacked to direct one-byte strings. Short strings are
handled directly in CSA, others need to call into C for conversion.

Microbenchmarks for "abcd".toLowerCase() show speedups of 2.5x.

BUG=v8:6353,v8:6344
==========

to

==========
[string] Move String.p.toLowerCase to CSA

This CL migrates the CPP builtin to CSA with fast paths for strings
that can be unpacked to direct one-byte strings. Short strings are
handled directly in CSA, others need to call into C for conversion.

Microbenchmarks for "abcd".toLowerCase() show speedups of 2.5x.

BUG=v8:6353,v8:6344

Review-Url: https://codereview.chromium.org/2859203002
Cr-Commit-Position: refs/heads/master@{#45141}
Committed:
https://chromium.googlesource.com/v8/v8/+/f0e95769dbc3c209f513c68e2aa7c9433c4...
==========

[commit-bot: I haz the power, 2017-05-05 15:59:18]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/v8/v8/+/f0e95769dbc3c209f513c68e2aa7c9433c4...