VM: Optimize RegExp.matchAsPrefix(...) by generating a sticky RegExp specialization.

VM: Optimize RegExp.matchAsPrefix(...) by generating a sticky RegExp specialization.

This is the same as a sticky RegExp flag in ES2015.

Overlay some RegExp fields on top of each other - given that they should never be used simultaneously.

BUG=http://dartbug.com/27810
R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/2403444eba54675d8b5d96912fce86f13eff1f1c

[Vyacheslav Egorov (Google), 2016-11-16 18:12:00]

Description was changed from

==========
VM: Optimize RegExp.matchAsPrefix(...) by generating a sticky RegExp
specialization.

This is the same as a sticky RegExp flag in ES2015.

BUG=http://dartbug.com/27810
==========

to

==========
VM: Optimize RegExp.matchAsPrefix(...) by generating a sticky RegExp
specialization.

This is the same as a sticky RegExp flag in ES2015.

Overlay some RegExp fields on top of each other - given that they should never
be used simultaneously. 

BUG=http://dartbug.com/27810
==========

[Vyacheslav Egorov (Google), 2016-11-16 18:12:00]

vegorov@google.com changed reviewers:
+ erikcorry@chromium.org, rmacnak@google.com

[Vyacheslav Egorov (Google), 2016-11-16 18:12:41]

PTAL

This improves matchAsPrefix performance for RegExp-s by at least a factor of 2.

[rmacnak, 2016-11-17 00:49:52]

lgtm

https://codereview.chromium.org/2510783002/diff/20001/runtime/lib/regexp.cc
File runtime/lib/regexp.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:99: // This function is intrinsified. See
Intrinsifier::RegExp_ExecuteMatch.
MatchSticky

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:2196: if (FLAG_interpret_irregexp ||
FLAG_precompiled_runtime) return;
Why add this? FLAG_interpret_irregexp should already be compile-time true in the
AOT runtime.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2897: // TODO(18854): Need to implement a way
of recreating the irrexp functions.
Remove this todo.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4859: // Add a .*? at the beginning, outside the body
capture, unless
Not sure I see why this property is called sticky, but I do see this would
improve prefix matching speed.

[erikcorry, 2016-11-17 13:56:16]

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

[erikcorry, 2016-11-17 13:56:17]

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:2195: void
Intrinsifier::RegExp_ExecuteMatch(Assembler* assembler) {
Can't RegExp_ExecuteMatch and RegExp_ExecuteMatchSticky not both call a helper
with a bool argument?

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2899: for (intptr_t cid = kOneByteStringCid;
cid <= kExternalTwoByteStringCid;
Here and elsewhere you put in a loop. Perhaps there should be kFirstStringCid
and kLastStringCid for use in loops, which are equal in value to the current
kOneByteStringCid and kExternalTwoByteStringCid.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4859: // Add a .*? at the beginning, outside the body
capture, unless
On 2016/11/17 00:49:52, rmacnak wrote:
> Not sure I see why this property is called sticky, but I do see this would
> improve prefix matching speed.

From es-discuss (!):

I believe the y actually comes from "yylex". ES4 proposals had originally called
for the corresponding property to be named anchored or nosearch before changing
it to sticky to avoid overloading the term anchored (which is commonly used to
describe regexes starting with ^ or ending with $) and to better fit with the
letter y. If the name changed to anchored, I'd prefer /a over /n. Flag n (as in
"(?n)") is already used by .NET for the ExplicitCapture option, which is a
feature that might be worth considering for ES6+. I don't mind "sticky",
personally.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4906:
macro_assembler->SetCurrentPositionFromEnd(max_length);
This needs fixing here too and we need a test for it.

[Vyacheslav Egorov (Google), 2016-11-17 16:46:23]

Description was changed from

==========
VM: Optimize RegExp.matchAsPrefix(...) by generating a sticky RegExp
specialization.

This is the same as a sticky RegExp flag in ES2015.

Overlay some RegExp fields on top of each other - given that they should never
be used simultaneously. 

BUG=http://dartbug.com/27810
==========

to

==========
VM: Optimize RegExp.matchAsPrefix(...) by generating a sticky RegExp
specialization.

This is the same as a sticky RegExp flag in ES2015.

Overlay some RegExp fields on top of each other - given that they should never
be used simultaneously.

BUG=http://dartbug.com/27810
R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2403444eba54675d8b5d96912fce86f13eff1f1c
==========

[Vyacheslav Egorov (Google), 2016-11-17 16:46:25]

Committed patchset #3 (id:40001) manually as
2403444eba54675d8b5d96912fce86f13eff1f1c (presubmit successful).

[Vyacheslav Egorov (Google), 2016-11-17 16:51:34]

Thanks for the reviews! Landing.

https://codereview.chromium.org/2510783002/diff/20001/runtime/lib/regexp.cc
File runtime/lib/regexp.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:99: // This function is intrinsified. See
Intrinsifier::RegExp_ExecuteMatch.
On 2016/11/17 00:49:52, rmacnak wrote:
> MatchSticky

Done.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:2195: void
Intrinsifier::RegExp_ExecuteMatch(Assembler* assembler) {
On 2016/11/17 13:56:17, erikcorry wrote:
> Can't RegExp_ExecuteMatch and RegExp_ExecuteMatchSticky not both call a helper
> with a bool argument?

In this case code below would have to check for boolean argument. It makes entry
into regexp slower.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:2196: if (FLAG_interpret_irregexp ||
FLAG_precompiled_runtime) return;
On 2016/11/17 00:49:52, rmacnak wrote:
> Why add this? FLAG_interpret_irregexp should already be compile-time true in
the
> AOT runtime.

There was a code somewhere that checked both so I added it here too to match. 

Instead I should remove it there.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2897: // TODO(18854): Need to implement a way
of recreating the irrexp functions.
On 2016/11/17 00:49:52, rmacnak wrote:
> Remove this todo.

Done.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2899: for (intptr_t cid = kOneByteStringCid;
cid <= kExternalTwoByteStringCid;
On 2016/11/17 13:56:17, erikcorry wrote:
> Here and elsewhere you put in a loop. Perhaps there should be kFirstStringCid
> and kLastStringCid for use in loops, which are equal in value to the current
> kOneByteStringCid and kExternalTwoByteStringCid.

Yeah, that sounds like a nice improvement - but I will not do that as a part of
this CL.

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/2510783002/diff/20001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4906:
macro_assembler->SetCurrentPositionFromEnd(max_length);
On 2016/11/17 13:56:17, erikcorry wrote:
> This needs fixing here too and we need a test for it.

As we discussed I have fixed it here and I will port a sticky test from V8.