[regexp] implement /ui to mirror the implementation for /i.

[regexp] implement /ui to mirror the implementation for /i.

R=erik.corry@gmail.com, erikcorry@chromium.org

Committed: https://crrev.com/eea1a4c003c559c99bcc9f08aa7eadf931975aad
Cr-Commit-Position: refs/heads/master@{#33655}

[Yang, 2016-01-27 10:22:55]

With this patch, the /ui handling mirrors the original /i handling more closely,
as it is implemented at the TextEmitPass level for BMP characters as well. This
allows us to emit quick checks as well as use trace.

However, I haven't been able to come up with a microbenchmark where this makes a
difference :/

[Yang, 2016-01-27 10:24:11]

Description was changed from

==========
[regexp] implement /ui similar to /i.

R=erik.corry@gmail.com, erikcorry@chromium.org
==========

to

==========
[regexp] implement /ui to mirror the implementation for /i.

R=erik.corry@gmail.com, erikcorry@chromium.org
==========

[Yang, 2016-02-01 04:52:57]

On 2016/01/27 10:22:55, Yang wrote:
> With this patch, the /ui handling mirrors the original /i handling more
closely,
> as it is implemented at the TextEmitPass level for BMP characters as well.
This
> allows us to emit quick checks as well as use trace.
> 
> However, I haven't been able to come up with a microbenchmark where this makes
a
> difference :/

Erik, do you have an opinion on this?

[erikcorry, 2016-02-01 15:21:39]

erikcorry@google.com changed reviewers:
+ erikcorry@google.com

[erikcorry, 2016-02-01 15:21:40]

lgtm

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc
File src/regexp/jsregexp.cc (left):

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#oldc...
src/regexp/jsregexp.cc:1660: if (one_byte && c > String::kMaxOneByteCharCodeU) {
I think this is still worth having.  For these characters we will emit an
unconditional backtrack elsewhere (they can never match on a one-byte subject
string) so it is just bloat to emit code here to load and check them.

We could do more to avoid outputting code in that case, but this looks like an
OK thing to have here.

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc
File src/regexp/jsregexp.cc (right):

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#newc...
src/regexp/jsregexp.cc:1592: bool one_byte_subject, uc32* letters) {
The compiler knows whether we have a one-byte subject, so we don't need that
argument any more.

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#newc...
src/regexp/jsregexp.cc:3256: // In the non-unicode case, if the pattern is
non-Latin1, it
I think you mean in the one-byte case (non-unicode sounds like you mean in the
absence of the /u flag, but that doesn't fit with the rest of the sentence).

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#newc...
src/regexp/jsregexp.cc:3362: if (compiler->one_byte()) {
You should check for the /ui flags here instead of inside TextEmitPass, since
there's no point stepping through the text, only to decide every time that
there's nothing to do.

[Yang, 2016-02-02 06:06:44]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-02 06:06:45]

The patchset sent to the CQ was uploaded after l-g-t-m from erikcorry@google.com
Link to the patchset: https://codereview.chromium.org/1641613002/#ps20001
(title: "addressed comment")

[Yang, 2016-02-02 06:06:48]

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc
File src/regexp/jsregexp.cc (left):

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#oldc...
src/regexp/jsregexp.cc:1660: if (one_byte && c > String::kMaxOneByteCharCodeU) {
On 2016/02/01 15:21:40, erikcorry wrote:
> I think this is still worth having.  For these characters we will emit an
> unconditional backtrack elsewhere (they can never match on a one-byte subject
> string) so it is just bloat to emit code here to load and check them.
> 
> We could do more to avoid outputting code in that case, but this looks like an
> OK thing to have here.

I'm confused here. Wouldn't the FilterOneByte pass remove this case before we
reach here, unless FilterOneByte gives up due to too deep recursion?

I put this line back just in case you disagree.

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc
File src/regexp/jsregexp.cc (right):

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#newc...
src/regexp/jsregexp.cc:1592: bool one_byte_subject, uc32* letters) {
On 2016/02/01 15:21:40, erikcorry wrote:
> The compiler knows whether we have a one-byte subject, so we don't need that
> argument any more.

Done.

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#newc...
src/regexp/jsregexp.cc:3256: // In the non-unicode case, if the pattern is
non-Latin1, it
On 2016/02/01 15:21:40, erikcorry wrote:
> I think you mean in the one-byte case (non-unicode sounds like you mean in the
> absence of the /u flag, but that doesn't fit with the rest of the sentence).

I did mean this. With /i, if a character is outside of the latin1 range, it
cannot match a character inside the latin1 range, so we had this optimization
here.
With /ui, we have to be careful not to miss the case where \u212b matches
\u00c5.

Changed the comment to be more clear about this.

https://codereview.chromium.org/1641613002/diff/1/src/regexp/jsregexp.cc#newc...
src/regexp/jsregexp.cc:3362: if (compiler->one_byte()) {
On 2016/02/01 15:21:40, erikcorry wrote:
> You should check for the /ui flags here instead of inside TextEmitPass, since
> there's no point stepping through the text, only to decide every time that
> there's nothing to do.

Done.

[commit-bot: I haz the power, 2016-02-02 06:06:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641613002/20001

[commit-bot: I haz the power, 2016-02-02 07:07:42]

Description was changed from

==========
[regexp] implement /ui to mirror the implementation for /i.

R=erik.corry@gmail.com, erikcorry@chromium.org
==========

to

==========
[regexp] implement /ui to mirror the implementation for /i.

R=erik.corry@gmail.com, erikcorry@chromium.org
==========

[commit-bot: I haz the power, 2016-02-02 07:07:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-02 07:09:48]

Description was changed from

==========
[regexp] implement /ui to mirror the implementation for /i.

R=erik.corry@gmail.com, erikcorry@chromium.org
==========

to

==========
[regexp] implement /ui to mirror the implementation for /i.

R=erik.corry@gmail.com, erikcorry@chromium.org

Committed: https://crrev.com/eea1a4c003c559c99bcc9f08aa7eadf931975aad
Cr-Commit-Position: refs/heads/master@{#33655}
==========

[commit-bot: I haz the power, 2016-02-02 07:09:49]

Patchset 2 (id:??) landed as
https://crrev.com/eea1a4c003c559c99bcc9f08aa7eadf931975aad
Cr-Commit-Position: refs/heads/master@{#33655}

[Yang, 2016-02-02 11:34:30]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1661483002/ by yangguo@chromium.org.

The reason for reverting is: This test fails:

assertEquals(["as"], /^a[\u017F]/ui.exec("as"));

The reason is that we end up with a character class that is not stand alone, so
we do not perform case folding on it correctly (with unicode flag)..