[regexp/string] Merge ExpandReplacement and GetSubstitution

[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=v8:5339
Committed: https://crrev.com/5ab3874559475479efa2b93f53bf508b71890497
Cr-Commit-Position: refs/heads/master@{#39528}

[jgruber, 2016-09-13 09:49:15]

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

[commit-bot: I haz the power, 2016-09-13 09:49:18]

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-09-13 09:53:51]

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

[commit-bot: I haz the power, 2016-09-13 09:53:51]

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

[jgruber, 2016-09-13 11:22:10]

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

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

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-09-13 11:25:22]

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

[commit-bot: I haz the power, 2016-09-13 11:25:23]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12620)

[jgruber, 2016-09-13 11:37:48]

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

[commit-bot: I haz the power, 2016-09-13 11:37:58]

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-09-13 11:39:49]

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

[commit-bot: I haz the power, 2016-09-13 11:39:50]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12622)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12616)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[jgruber, 2016-09-13 11:50:11]

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

[commit-bot: I haz the power, 2016-09-13 11:50:18]

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-09-13 11:51:48]

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

[commit-bot: I haz the power, 2016-09-13 11:51:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12624)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12556)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23913)

[jgruber, 2016-09-13 12:51:35]

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

[commit-bot: I haz the power, 2016-09-13 12:51:47]

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-09-13 13:15:55]

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

[commit-bot: I haz the power, 2016-09-13 13:15:56]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-09-13 17:14:53]

It's definitely nicer-looking code, but does this patch cause any performance
regression?

[Dan Ehrenberg, 2016-09-13 17:28:20]

On 2016/09/13 at 17:14:53, Dan Ehrenberg wrote:
> It's definitely nicer-looking code, but does this patch cause any performance
regression?

For this, I think it'd be nice to do a replacement string microbenchmark locally
to be sure.

[jgruber, 2016-09-14 07:18:31]

Description was changed from

==========
[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=
==========

to

==========
[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=v8:5339
==========

[jgruber, 2016-09-14 08:44:30]

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

[commit-bot: I haz the power, 2016-09-14 08:44:34]

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-14 08:47:06]

On 2016/09/13 17:28:20, Dan Ehrenberg wrote:
> On 2016/09/13 at 17:14:53, Dan Ehrenberg wrote:
> > It's definitely nicer-looking code, but does this patch cause any
performance
> regression?
> 
> For this, I think it'd be nice to do a replacement string microbenchmark
locally
> to be sure.

I tweaked the implementation a bit and we're now at around a 3% regression on a
local replace microbenchmark (octane/regexp modified to only run replace
operations). See also perf bot results on this CL.

[commit-bot: I haz the power, 2016-09-14 08:59:04]

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

[commit-bot: I haz the power, 2016-09-14 08:59:05]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8815)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[jgruber, 2016-09-14 11:56:40]

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

[commit-bot: I haz the power, 2016-09-14 11:56:47]

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-09-14 12:40:51]

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

[commit-bot: I haz the power, 2016-09-14 12:40:52]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-16 07:28:27]

ptal. Seems like the trade-off is ~2% performance loss on replace vs. more
maintainable code.

[Dan Ehrenberg, 2016-09-19 15:33:07]

On 2016/09/16 at 07:28:27, jgruber wrote:
> ptal. Seems like the trade-off is ~2% performance loss on replace vs. more
maintainable code.

The code looks good to me; could you add another reviewer to determine whether
this regression is acceptable?

[jgruber, 2016-09-20 07:17:29]

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

[jgruber, 2016-09-20 07:17:31]

+bmeurer for opinions on 2% regression for Symbol.replace.

[Benedikt Meurer, 2016-09-20 07:24:48]

LGTM on 2%

[Benedikt Meurer, 2016-09-20 07:25:21]

On 2016/09/20 07:24:48, Benedikt Meurer wrote:
> LGTM on 2%

LGTM on code

[jgruber, 2016-09-20 07:43:48]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2016-09-20 07:43:54]

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-09-20 08:13:27]

Description was changed from

==========
[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=v8:5339
==========

to

==========
[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=v8:5339
==========

[commit-bot: I haz the power, 2016-09-20 08:13:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-20 08:13:41]

Description was changed from

==========
[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=v8:5339
==========

to

==========
[regexp/string] Merge ExpandReplacement and GetSubstitution

R=littledan@chromium.org
BUG=v8:5339

Committed: https://crrev.com/5ab3874559475479efa2b93f53bf508b71890497
Cr-Commit-Position: refs/heads/master@{#39528}
==========

[commit-bot: I haz the power, 2016-09-20 08:13:42]

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