Add ES2015 RegExp full subclassing semantics behind a flag

Add ES2015 RegExp full subclassing semantics behind a flag

This patch implements ES2015 RegExp subclassing semantics, namely the
hardest part where RegExp.prototype.exec and certain flag getters can
be overridden in order to provide different behavior. This change is
hidden behind a new flag, --harmony-regexp-exec. The flag guards the
behavior by installing entirely different implementations of the
methods which follow the new semantics.

Preliminary performance tests show a 3-4x regression in the Octane
RegExp benchmark. The new code doesn't call out into several fast
paths that the old code supported, so this is expected.

The patch is tested mostly by test262, where most RegExp tests are fixed,
with the exception of deliberate spec violations for web compatibility,
and for the 'sticky' flag, which is not dynamically read by this patch
in all cases but rather statically compiled into the RegExp. The latter
will require a follow-on patch to implement. A small additional set of
tests verifies one particular case, mostly to check whether the flag
mechanism works.

R=adamk,yangguo@chromium.org
LOG=Y
BUG=v8:4602
Committed: https://crrev.com/92a571e5460b7abb53105e13721beeaac7fd085a
Cr-Commit-Position: refs/heads/master@{#35068}

[Dan Ehrenberg, 2016-03-04 23:41:26]

Description was changed from

==========
Infrastructure for RegExp subclassing behind a flag, and partial implementation

---stacked on top of---

Separate String.prototype.replace into RegExp.prototype[Symbol.replace]

This patch implements one aspect of ES2015 RegExp subclassing:
String.prototype.replace is separated into two parts, a method on
RegExp.prototype in case the first argument is a RegExp, and the
String.prototype.replace method, which handles the string pattern
case. This separation is described in the ES2015 specification.
Most of the patch is simply moving code from string.js to regexp.js.

R=yangguo
LOG=Y
BUG=v8:4343
==========

to

==========
Infrastructure for RegExp subclassing behind a flag, and partial implementation

R=yangguo
LOG=Y
BUG=v8:4343
==========

[Yang, 2016-03-07 06:56:20]

On 2016/03/04 23:41:26, Dan Ehrenberg wrote:
> Description was changed from
> 
> ==========
> Infrastructure for RegExp subclassing behind a flag, and partial
implementation
> 
> ---stacked on top of---
> 
> Separate String.prototype.replace into RegExp.prototype[Symbol.replace]
> 
> This patch implements one aspect of ES2015 RegExp subclassing:
> String.prototype.replace is separated into two parts, a method on
> RegExp.prototype in case the first argument is a RegExp, and the
> String.prototype.replace method, which handles the string pattern
> case. This separation is described in the ES2015 specification.
> Most of the patch is simply moving code from string.js to regexp.js.
> 
> R=yangguo
> LOG=Y
> BUG=v8:4343
> ==========
> 
> to
> 
> ==========
> Infrastructure for RegExp subclassing behind a flag, and partial
implementation
> 
> R=yangguo
> LOG=Y
> BUG=v8:4343
> ==========

I'm a bit confused. Is this up for review?

[Dan Ehrenberg, 2016-03-07 15:36:17]

littledan@chromium.org changed reviewers:
+ yangguo@chromium.org
- yangguo@google.com

[Dan Ehrenberg, 2016-03-07 15:36:55]

On 2016/03/07 at 06:56:20, yangguo wrote:
> On 2016/03/04 23:41:26, Dan Ehrenberg wrote:
> > Description was changed from
> > 
> > ==========
> > Infrastructure for RegExp subclassing behind a flag, and partial
implementation
> > 
> > ---stacked on top of---
> > 
> > Separate String.prototype.replace into RegExp.prototype[Symbol.replace]
> > 
> > This patch implements one aspect of ES2015 RegExp subclassing:
> > String.prototype.replace is separated into two parts, a method on
> > RegExp.prototype in case the first argument is a RegExp, and the
> > String.prototype.replace method, which handles the string pattern
> > case. This separation is described in the ES2015 specification.
> > Most of the patch is simply moving code from string.js to regexp.js.
> > 
> > R=yangguo
> > LOG=Y
> > BUG=v8:4343
> > ==========
> > 
> > to
> > 
> > ==========
> > Infrastructure for RegExp subclassing behind a flag, and partial
implementation
> > 
> > R=yangguo
> > LOG=Y
> > BUG=v8:4343
> > ==========
> 
> I'm a bit confused. Is this up for review?

No, just drafting it here so I can use the precq

[Dan Ehrenberg, 2016-03-07 21:52:40]

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

[commit-bot: I haz the power, 2016-03-07 21:52:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/140001

[commit-bot: I haz the power, 2016-03-07 21:54:26]

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

[commit-bot: I haz the power, 2016-03-07 21:54:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2499)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/2508)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/16619)

[Dan Ehrenberg, 2016-03-22 00:24:14]

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

[commit-bot: I haz the power, 2016-03-22 00:24:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/160001

[Dan Ehrenberg, 2016-03-22 00:24:40]

Description was changed from

==========
Infrastructure for RegExp subclassing behind a flag, and partial implementation

R=yangguo
LOG=Y
BUG=v8:4343
==========

to

==========
Add ES2015 RegExp full subclassing semantics behind a flag

This patch implements ES2015 RegExp subclassing semantics, namely the
hardest part where RegExp.prototype.exec and certain flag getters can
be overridden in order to provide different behavior. This change is
hidden behind a new flag, --harmony-regexp-exec. The flag guards the
behavior by installing entirely different implementations of the
methods which follow the new semantics.

Preliminary performance tests show a 3-4x regression in the Octane
RegExp benchmark.

The patch is tested mostly by test262, where most RegExp tests are fixed,
with the exception of deliberate spec violations for web compatibility,
and for the 'sticky' flag, which is not dynamically read by this patch
in all cases but rather statically compiled into the RegExp. The latter
will require a follow-on patch to implement. A small additional set of
tests verifies one particular case, mostly to check whether the flag
mechanism works.

R=adamk,yangguo@chromium.org
LOG=Y
BUG=v8:4602
==========

[Dan Ehrenberg, 2016-03-22 00:24:40]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org

[commit-bot: I haz the power, 2016-03-22 00:25:31]

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

[commit-bot: I haz the power, 2016-03-22 00:25:32]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/3291)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/17413)

[Dan Ehrenberg, 2016-03-22 00:26:29]

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

[commit-bot: I haz the power, 2016-03-22 00:26:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/180001

[commit-bot: I haz the power, 2016-03-22 00:30:29]

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

[commit-bot: I haz the power, 2016-03-22 00:30:30]

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

[Dan Ehrenberg, 2016-03-22 00:44:02]

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

[commit-bot: I haz the power, 2016-03-22 00:44:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/200001

[Dan Ehrenberg, 2016-03-22 00:48:56]

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

[commit-bot: I haz the power, 2016-03-22 00:49:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/220001

[Dan Ehrenberg, 2016-03-22 00:56:40]

Description was changed from

==========
Add ES2015 RegExp full subclassing semantics behind a flag

This patch implements ES2015 RegExp subclassing semantics, namely the
hardest part where RegExp.prototype.exec and certain flag getters can
be overridden in order to provide different behavior. This change is
hidden behind a new flag, --harmony-regexp-exec. The flag guards the
behavior by installing entirely different implementations of the
methods which follow the new semantics.

Preliminary performance tests show a 3-4x regression in the Octane
RegExp benchmark.

The patch is tested mostly by test262, where most RegExp tests are fixed,
with the exception of deliberate spec violations for web compatibility,
and for the 'sticky' flag, which is not dynamically read by this patch
in all cases but rather statically compiled into the RegExp. The latter
will require a follow-on patch to implement. A small additional set of
tests verifies one particular case, mostly to check whether the flag
mechanism works.

R=adamk,yangguo@chromium.org
LOG=Y
BUG=v8:4602
==========

to

==========
Add ES2015 RegExp full subclassing semantics behind a flag

This patch implements ES2015 RegExp subclassing semantics, namely the
hardest part where RegExp.prototype.exec and certain flag getters can
be overridden in order to provide different behavior. This change is
hidden behind a new flag, --harmony-regexp-exec. The flag guards the
behavior by installing entirely different implementations of the
methods which follow the new semantics.

Preliminary performance tests show a 3-4x regression in the Octane
RegExp benchmark. The new code doesn't call out into several fast
paths that the old code supported, so this is expected.

The patch is tested mostly by test262, where most RegExp tests are fixed,
with the exception of deliberate spec violations for web compatibility,
and for the 'sticky' flag, which is not dynamically read by this patch
in all cases but rather statically compiled into the RegExp. The latter
will require a follow-on patch to implement. A small additional set of
tests verifies one particular case, mostly to check whether the flag
mechanism works.

R=adamk,yangguo@chromium.org
LOG=Y
BUG=v8:4602
==========

[commit-bot: I haz the power, 2016-03-22 01:02:19]

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

[commit-bot: I haz the power, 2016-03-22 01:02:21]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)

[Dan Ehrenberg, 2016-03-22 01:21:11]

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

[commit-bot: I haz the power, 2016-03-22 01:21:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/240001

[adamk, 2016-03-22 01:26:38]

As noted offline it would be helpful to have a pointer to which spec bits these
functions implement.

Will review in more detail once those comments are present.

https://codereview.chromium.org/1596483005/diff/220001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/220001/src/js/regexp.js#newco...
src/js/regexp.js:24: var MathMin = global.Math.min;
You can use MaxSimple/MinSimple here instead.

https://codereview.chromium.org/1596483005/diff/220001/src/js/regexp.js#newco...
src/js/regexp.js:176: function RegExpSubclassExecJS(string) {
I take it these functions are in here so as to avoid slowing down context
creation? Or is there some other reason?

[commit-bot: I haz the power, 2016-03-22 01:40:12]

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

[commit-bot: I haz the power, 2016-03-22 01:40:13]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-03-22 12:49:38]

https://codereview.chromium.org/1596483005/diff/240001/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/1596483005/diff/240001/src/js/prologue.js#new...
src/js/prologue.js:136: if (!afterInitialBootstrap) %FunctionRemovePrototype(f);
Let's omit this optimization. I'm not convinced we need to speed up boostrapping
harmony scripts like this.

https://codereview.chromium.org/1596483005/diff/240001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/240001/src/js/regexp.js#newco...
src/js/regexp.js:218: %FunctionRemovePrototype(RegExpSubclassExecJS);
Like previously noted, let's always do this in OverrideFunction.

https://codereview.chromium.org/1596483005/diff/240001/src/js/regexp.js#newco...
src/js/regexp.js:1056: 
stray edit?

[Dan Ehrenberg, 2016-03-22 18:26:31]

Included better cross-referencing comments to the spec.

https://codereview.chromium.org/1596483005/diff/220001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/220001/src/js/regexp.js#newco...
src/js/regexp.js:24: var MathMin = global.Math.min;
On 2016/03/22 at 01:26:38, adamk wrote:
> You can use MaxSimple/MinSimple here instead.

Done

https://codereview.chromium.org/1596483005/diff/220001/src/js/regexp.js#newco...
src/js/regexp.js:176: function RegExpSubclassExecJS(string) {
On 2016/03/22 at 01:26:38, adamk wrote:
> I take it these functions are in here so as to avoid slowing down context
creation? Or is there some other reason?

Yes, based on my experience with Array and TypedArray methods, it seemed like we
wouldn't be able to turn this flag on unless the functions are implemented in
this file.

https://codereview.chromium.org/1596483005/diff/240001/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/1596483005/diff/240001/src/js/prologue.js#new...
src/js/prologue.js:136: if (!afterInitialBootstrap) %FunctionRemovePrototype(f);
On 2016/03/22 at 12:49:38, Yang wrote:
> Let's omit this optimization. I'm not convinced we need to speed up
boostrapping harmony scripts like this.

This isn't an optimization--apparently you can't call this after bootstrap is
over without crashing.

https://codereview.chromium.org/1596483005/diff/240001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/240001/src/js/regexp.js#newco...
src/js/regexp.js:218: %FunctionRemovePrototype(RegExpSubclassExecJS);
On 2016/03/22 at 12:49:38, Yang wrote:
> Like previously noted, let's always do this in OverrideFunction.

See v8:4567 for the bug that this is working around.

https://codereview.chromium.org/1596483005/diff/240001/src/js/regexp.js#newco...
src/js/regexp.js:1056: 
On 2016/03/22 at 12:49:38, Yang wrote:
> stray edit?

Reverted

[Dan Ehrenberg, 2016-03-22 18:44:55]

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

[commit-bot: I haz the power, 2016-03-22 18:45:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/300001

[commit-bot: I haz the power, 2016-03-22 19:14:41]

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

[commit-bot: I haz the power, 2016-03-22 19:14:42]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-03-22 22:23:42]

Some comments, can't guarantee this is everything (lots of new code there,
though it does match almost entirely to the spec).

How well tested is the new behavior, by test262?

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:197: var global = TO_BOOLEAN(this.global);
Are these the only calls that differ from the existing RegExpExec?

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:201: if (i < 0 || i > string.length) {
I see where the > length is checked in the spec, but where's the < 0? Also, >
length doesn't appear to depend on global || sticky.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:490: var array = new GlobalArray();
It seems like the main reason to use an Array here instead of an InternalArray
is because of the multiple returns? I wonder if it would be faster to use an
InternalArray and avoid all the calls to AddIndexedProperty.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:510: var end = MinSimple(splitter.lastIndex, size);
Missing TO_LENGTH? Hard to tell it's required, but I see it there in the spec.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:577: if (matchStr === "") AdvanceStringIndex(this, string,
unicode);
I found it quite confusing that what the spec calls "AdvanceStringIndex" you
call "GetUnicodeAdvancedIncrement", and then AdvanceStringIndex here means
something else.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:757: // TODO(littledan): Call this function from
String.prototype.replace instead
I agree :)

This one's really big; what's the difference with the string one? Is there any
hope of merging them while this is still under a flag?

[Dan Ehrenberg, 2016-03-22 22:58:41]

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

[commit-bot: I haz the power, 2016-03-22 22:58:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/320001

[Dan Ehrenberg, 2016-03-22 23:09:32]

I think this behavior is pretty well-tested in test262. In addition to our
pretty extensive RegExp tests that we already had (which all passed), test262
has 80 that failed before this patch that pass with the flag turned on. I
haven't surveyed them individually myself, but it looks like test262 gave this
part of the spec the test-every-line treatment; the failures look pretty
diverse.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:197: var global = TO_BOOLEAN(this.global);
On 2016/03/22 at 22:23:42, adamk wrote:
> Are these the only calls that differ from the existing RegExpExec?

Yes. They showed up as relatively big items in the profile, so I'd like them to
start out behind the flag. Maybe we can consolidate the other code though.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:201: if (i < 0 || i > string.length) {
On 2016/03/22 at 22:23:42, adamk wrote:
> I see where the > length is checked in the spec, but where's the < 0? Also, >
length doesn't appear to depend on global || sticky.

My mistake, the < 0 path should be unreachable now that we're calling ToLength.
The spec says

7. If global is false and sticky is false, let lastIndex be 0.

so it's impossible for i > string.length, since string lengths are all >= 0.

This code would probably benefit from a cleanup of removing "i < 0", so I did
that.

As far as making it conditional on updateLastIndex: The bigger reason for that
conditional is to set i = 0 when it's false. It's basically an optimization to
do the early-null-return here rather than after RegExpExec fails; I decided not
to remove that optimization since it carried over just fine. You don't see
corresponding spec language because the spec only implements the check in lines
215-218.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:490: var array = new GlobalArray();
On 2016/03/22 at 22:23:42, adamk wrote:
> It seems like the main reason to use an Array here instead of an InternalArray
is because of the multiple returns? I wonder if it would be faster to use an
InternalArray and avoid all the calls to AddIndexedProperty.

Good idea for optimization; I was just blindly following the spec here. Is this
OK to leave for a follow-on optimization patch?

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:510: var end = MinSimple(splitter.lastIndex, size);
On 2016/03/22 at 22:23:42, adamk wrote:
> Missing TO_LENGTH? Hard to tell it's required, but I see it there in the spec.

Oops, definitely needed around splitter.lastIndex. Fixed.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:577: if (matchStr === "") AdvanceStringIndex(this, string,
unicode);
On 2016/03/22 at 22:23:42, adamk wrote:
> I found it quite confusing that what the spec calls "AdvanceStringIndex" you
call "GetUnicodeAdvancedIncrement", and then AdvanceStringIndex here means
something else.

Fixed it to have more meaningful names.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:757: // TODO(littledan): Call this function from
String.prototype.replace instead
On 2016/03/22 at 22:23:42, adamk wrote:
> I agree :)
> 
> This one's really big; what's the difference with the string one? Is there any
hope of merging them while this is still under a flag?

They take the captures/replacement in different formats (actual substrings vs
indices). I don't have a great idea for how to unify them without threatening
the performance of the shipped version; my only idea was to use
ExpandReplacement when the protector is doing well.

[commit-bot: I haz the power, 2016-03-22 23:31:18]

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

[commit-bot: I haz the power, 2016-03-22 23:31:19]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-03-23 01:07:19]

Looking better. Per offline discussion, it sounds like the idea is that this is
just step 1 of N in getting a fast-enough version of this code. As-is it's
designed for maximum spec-mirroring, which seems like the right way to start
(especially given it passes all existing tests with the flag on).

I'd like Yang to take a close look too.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:488: var new_flags = sticky ? flags : flags + "y";
I'd prefer if new code added in this patch used camelCased style consistently,
as that's what we generally try to do in JS code. Not a deal-breaker, but my eye
definitely notices it.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:490: var array = new GlobalArray();
On 2016/03/22 23:09:31, Dan Ehrenberg wrote:
> On 2016/03/22 at 22:23:42, adamk wrote:
> > It seems like the main reason to use an Array here instead of an
InternalArray
> is because of the multiple returns? I wonder if it would be faster to use an
> InternalArray and avoid all the calls to AddIndexedProperty.
> 
> Good idea for optimization; I was just blindly following the spec here. Is
this
> OK to leave for a follow-on optimization patch?

Given that we already know there's lots of optimization to do, that's ok with
me.

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:918: replacement = GetSubstitution(matched, string, position,
captures, replace);
Nit: 80 cols.

[Dan Ehrenberg, 2016-03-24 00:50:54]

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:488: var new_flags = sticky ? flags : flags + "y";
On 2016/03/23 at 01:07:19, adamk wrote:
> I'd prefer if new code added in this patch used camelCased style consistently,
as that's what we generally try to do in JS code. Not a deal-breaker, but my eye
definitely notices it.

fixed

https://codereview.chromium.org/1596483005/diff/280001/src/js/regexp.js#newco...
src/js/regexp.js:918: replacement = GetSubstitution(matched, string, position,
captures, replace);
On 2016/03/23 at 01:07:19, adamk wrote:
> Nit: 80 cols.

fixed

[Dan Ehrenberg, 2016-03-24 00:51:06]

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

[commit-bot: I haz the power, 2016-03-24 00:51:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596483005/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596483005/360001

[commit-bot: I haz the power, 2016-03-24 01:14:46]

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

[commit-bot: I haz the power, 2016-03-24 01:14:47]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-03-24 21:38:08]

lgtm % a few more camelCase-needing functions

Let's get this landed (it's all flag-guarded anyway) so we can start work on
optimization.

https://codereview.chromium.org/1596483005/diff/360001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:758: var match_length = matched.length;
Style: matchLength, and throughout this function

https://codereview.chromium.org/1596483005/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:872: var functional_replace = IS_CALLABLE(replace);
And here too

[Dan Ehrenberg, 2016-03-24 21:55:08]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-03-24 21:55:10]

https://codereview.chromium.org/1596483005/diff/360001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1596483005/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:758: var match_length = matched.length;
On 2016/03/24 at 21:38:08, adamk wrote:
> Style: matchLength, and throughout this function

Done

https://codereview.chromium.org/1596483005/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:872: var functional_replace = IS_CALLABLE(replace);
On 2016/03/24 at 21:38:08, adamk wrote:
> And here too

Done

[Dan Ehrenberg, 2016-03-24 21:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1596483005/#ps400001
(title: "camelCase")

[commit-bot: I haz the power, 2016-03-24 21:55:16]

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

[commit-bot: I haz the power, 2016-03-24 22:26:23]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-03-24 22:27:31]

Description was changed from

==========
Add ES2015 RegExp full subclassing semantics behind a flag

This patch implements ES2015 RegExp subclassing semantics, namely the
hardest part where RegExp.prototype.exec and certain flag getters can
be overridden in order to provide different behavior. This change is
hidden behind a new flag, --harmony-regexp-exec. The flag guards the
behavior by installing entirely different implementations of the
methods which follow the new semantics.

Preliminary performance tests show a 3-4x regression in the Octane
RegExp benchmark. The new code doesn't call out into several fast
paths that the old code supported, so this is expected.

The patch is tested mostly by test262, where most RegExp tests are fixed,
with the exception of deliberate spec violations for web compatibility,
and for the 'sticky' flag, which is not dynamically read by this patch
in all cases but rather statically compiled into the RegExp. The latter
will require a follow-on patch to implement. A small additional set of
tests verifies one particular case, mostly to check whether the flag
mechanism works.

R=adamk,yangguo@chromium.org
LOG=Y
BUG=v8:4602
==========

to

==========
Add ES2015 RegExp full subclassing semantics behind a flag

This patch implements ES2015 RegExp subclassing semantics, namely the
hardest part where RegExp.prototype.exec and certain flag getters can
be overridden in order to provide different behavior. This change is
hidden behind a new flag, --harmony-regexp-exec. The flag guards the
behavior by installing entirely different implementations of the
methods which follow the new semantics.

Preliminary performance tests show a 3-4x regression in the Octane
RegExp benchmark. The new code doesn't call out into several fast
paths that the old code supported, so this is expected.

The patch is tested mostly by test262, where most RegExp tests are fixed,
with the exception of deliberate spec violations for web compatibility,
and for the 'sticky' flag, which is not dynamically read by this patch
in all cases but rather statically compiled into the RegExp. The latter
will require a follow-on patch to implement. A small additional set of
tests verifies one particular case, mostly to check whether the flag
mechanism works.

R=adamk,yangguo@chromium.org
LOG=Y
BUG=v8:4602

Committed: https://crrev.com/92a571e5460b7abb53105e13721beeaac7fd085a
Cr-Commit-Position: refs/heads/master@{#35068}
==========

[commit-bot: I haz the power, 2016-03-24 22:27:33]

Patchset 21 (id:??) landed as
https://crrev.com/92a571e5460b7abb53105e13721beeaac7fd085a
Cr-Commit-Position: refs/heads/master@{#35068}