[regexp] Port RegExpConstructor to C++

[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

That's probably only the case because exec and replace dominate
octane/regexp. There's potential for improvement here, for instance by
adding a fast-path if new.target is an unmodified JSRegExp function.

BUG=v8:5339
Committed: https://crrev.com/d515156441fcdbb981fb6fde03bc6a72ce40ea72
Cr-Commit-Position: refs/heads/master@{#39981}

[jgruber, 2016-09-30 11:13:20]

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

[commit-bot: I haz the power, 2016-09-30 11:13:22]

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

[jgruber, 2016-09-30 11:15:12]

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

[Benedikt Meurer, 2016-09-30 11:25:24]

https://codereview.chromium.org/2384613004/diff/1/src/builtins/builtins-regex...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2384613004/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp.cc:111: if (*pattern_constructor == *new_target) {
Nit: use is_identical_to here

https://codereview.chromium.org/2384613004/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp.cc:115: } else if (!new_target->IsJSReceiver()) {
Shouldn't this be IsCallable? Because new_target can either be JSReceiver or
Undefined, so this is always true otherwise.

[commit-bot: I haz the power, 2016-09-30 11:41:57]

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

[commit-bot: I haz the power, 2016-09-30 11:41:58]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-10-04 12:25:22]

PTAL

https://codereview.chromium.org/2384613004/diff/1/src/builtins/builtins-regex...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2384613004/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp.cc:111: if (*pattern_constructor == *new_target) {
On 2016/09/30 11:25:24, Benedikt Meurer wrote:
> Nit: use is_identical_to here

Done.

https://codereview.chromium.org/2384613004/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp.cc:115: } else if (!new_target->IsJSReceiver()) {
On 2016/09/30 11:25:24, Benedikt Meurer wrote:
> Shouldn't this be IsCallable? Because new_target can either be JSReceiver or
> Undefined, so this is always true otherwise.

I had a response to this all written out saying new_target could be e.g. a Smi
here through Reflect.construct, but then I figured out that Reflect.construct
ensures new_target is actually a constructor.

Removing this if branch.

[jgruber, 2016-10-04 14:43:36]

Description was changed from

==========
[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

BUG=v8:5339
==========

to

==========
[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

That's probably only the case because exec and replace dominate 
octane/regexp. There's potential for improvement here, for instance by 
adding a fast-path if new.target is an unmodified JSRegExp function.

BUG=v8:5339
==========

[jgruber, 2016-10-04 14:43:36]

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

[Benedikt Meurer, 2016-10-05 05:53:29]

bmeurer@chromium.org changed required reviewers:
+ yangguo@chromium.org

[Benedikt Meurer, 2016-10-05 05:53:30]

Nice, LGTM from my side.

[Yang, 2016-10-05 07:40:34]

LGTM. Nice.

https://codereview.chromium.org/2384613004/diff/60001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2384613004/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:42: IncrementalStringBuilder builder(isolate);
Using the IncrementalStringBuilder is a bit overkill here. We have at most 5
characters and it's definitely always a one-byte string. We could just allocate
a char[5] on the stack and pass that to Factory::NewStringFromOneByte.

The IncrementalStringBuilder would always allocate a 32-character string on the
heap, check for the length on every Append, and truncate the string at the end.
We could extend it to allocate arbitrary sizes at the start (5 in our cases) and
use the IncrementalStringBuilder::NoExtend to avoid the checks, but that seems
over-enginieering it too.

Using NewStringFromOneByte also has the benefit that we would canonicalize
single-character strings.

Then again, all of this probably does not affect performance a lot. And could be
done in a separate CL.

[jgruber, 2016-10-05 07:58:59]

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

[commit-bot: I haz the power, 2016-10-05 07:59:02]

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

[jgruber, 2016-10-05 07:59:51]

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

[commit-bot: I haz the power, 2016-10-05 08:00:02]

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, 2016-10-05 08:30:36]

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

[commit-bot: I haz the power, 2016-10-05 08:30:37]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-10-05 08:37:03]

https://codereview.chromium.org/2384613004/diff/60001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2384613004/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:42: IncrementalStringBuilder builder(isolate);
On 2016/10/05 07:40:34, Yang wrote:
> Using the IncrementalStringBuilder is a bit overkill here. We have at most 5
> characters and it's definitely always a one-byte string. We could just
allocate
> a char[5] on the stack and pass that to Factory::NewStringFromOneByte.
> 
> The IncrementalStringBuilder would always allocate a 32-character string on
the
> heap, check for the length on every Append, and truncate the string at the
end.
> We could extend it to allocate arbitrary sizes at the start (5 in our cases)
and
> use the IncrementalStringBuilder::NoExtend to avoid the checks, but that seems
> over-enginieering it too.
> 
> Using NewStringFromOneByte also has the benefit that we would canonicalize
> single-character strings.
> 
> Then again, all of this probably does not affect performance a lot. And could
be
> done in a separate CL.

Good point, done. We could improve this even more in the future by not
converting flags back and forth between the bitmap and string representation in
some cases.

[jgruber, 2016-10-05 08:42:53]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-10-05 08:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2384613004/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-05 08:42:57]

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, 2016-10-05 09:13:06]

Description was changed from

==========
[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

That's probably only the case because exec and replace dominate 
octane/regexp. There's potential for improvement here, for instance by 
adding a fast-path if new.target is an unmodified JSRegExp function.

BUG=v8:5339
==========

to

==========
[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

That's probably only the case because exec and replace dominate 
octane/regexp. There's potential for improvement here, for instance by 
adding a fast-path if new.target is an unmodified JSRegExp function.

BUG=v8:5339
==========

[commit-bot: I haz the power, 2016-10-05 09:13:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-05 09:13:46]

Description was changed from

==========
[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

That's probably only the case because exec and replace dominate 
octane/regexp. There's potential for improvement here, for instance by 
adding a fast-path if new.target is an unmodified JSRegExp function.

BUG=v8:5339
==========

to

==========
[regexp] Port RegExpConstructor to C++

This moves the RegExp constructor to C++. Local runs of octane/regexp are
performance-neutral:

C++: 4970.1 +- 404.981
JS: 4869.2 +- 586.743

That's probably only the case because exec and replace dominate
octane/regexp. There's potential for improvement here, for instance by
adding a fast-path if new.target is an unmodified JSRegExp function.

BUG=v8:5339

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

[commit-bot: I haz the power, 2016-10-05 09:13:47]

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