New issue
Advanced search Search tips
Starred by 8 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
HW: All
NextAction: 2018-12-10
OS: All
Priority: 2
Type: FeatureRequest

Blocked on:
issue 8574



Sign in to add a comment
link

Issue 6890: Implement String.prototype.matchAll

Reported by mathias@chromium.org, Oct 5 2017 Project Member

Issue description

Comment 1 by mathias@chromium.org, Oct 6 2017

Description: Show this description

Comment 2 by jgruber@chromium.org, Dec 13 2017

Cc: peter.wm...@gmail.com
Labels: HW-All OS-All Priority-2
As this is coming closer to stage 3 we should look into implementing this soonish.

Comment 3 by jgruber@chromium.org, Dec 14 2017

Cc: -peter.wm...@gmail.com jgruber@chromium.org
Owner: peter.wm...@gmail.com

Comment 4 by peter.wm...@gmail.com, Mar 28 2018

Update #1: Initial CSA implementation is up for review: https://chromium-review.googlesource.com/c/v8/v8/+/981893

Update #2: Tests have cleared initial review from the proposal champion (Jordan Harband) and has been submitted to Test262, but is pending possible spec update: https://github.com/tc39/test262/pull/1500

Comment 5 by peter.wm...@gmail.com, Mar 28 2018

Status: Started (was: Assigned)

Comment 6 by bugdroid1@chromium.org, Apr 5 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03

commit 3b39fc4dcdb6593013c497fc9e28a1d73dbcba03
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Thu Apr 05 15:24:25 2018

[esnext] Implement String.prototype.matchAll

Proposal repo: https://github.com/tc39/proposal-string-matchall

- Add new builtins StringPrototypeMatchAll and RegExpPrototypeMatchAll
- Add new object RegExpStringIterator

Bug: v8:6890
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: I9fad71900cf30e8632258c309df1c7a638ea4600
Reviewed-on: https://chromium-review.googlesource.com/981893
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52403}
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/BUILD.gn
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/bootstrapper.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/builtins/builtins-definitions.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/builtins/builtins-regexp-gen.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/builtins/builtins-string-gen.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/compiler/code-assembler.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/compiler/types.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/contexts.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/flag-definitions.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/heap-symbols.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects-body-descriptors-inl.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects-debug.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects-inl.h
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects-printer.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects.cc
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects.h
[add] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects/js-regexp-string-iterator-inl.h
[add] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/src/objects/js-regexp-string-iterator.h
[add] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/test/mjsunit/harmony/string-matchAll.js
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/test/test262/testcfg.py
[modify] https://crrev.com/3b39fc4dcdb6593013c497fc9e28a1d73dbcba03/tools/v8heapconst.py

Comment 7 by bugdroid1@chromium.org, Apr 10 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b4cf629812b721225fd9e3fc923dd0b97f90d9e6

commit b4cf629812b721225fd9e3fc923dd0b97f90d9e6
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Tue Apr 10 12:47:33 2018

[js-perf-test] Add benchmark for String.prototype.matchAll

Bug: v8:6890
Change-Id: I0778aee65985852950c48b519baeb7fe6d81f8eb
Reviewed-on: https://chromium-review.googlesource.com/998394
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52520}
[modify] https://crrev.com/b4cf629812b721225fd9e3fc923dd0b97f90d9e6/test/js-perf-test/JSTests.json
[add] https://crrev.com/b4cf629812b721225fd9e3fc923dd0b97f90d9e6/test/js-perf-test/Strings/string-matchall.js

Comment 8 by bugdroid1@chromium.org, Apr 12 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/ae4529f9e1e635d49364c9e62801d067dc806a95

commit ae4529f9e1e635d49364c9e62801d067dc806a95
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Thu Apr 12 16:13:44 2018

[builtins] Add fast paths to String.p.matchAll

Add fast paths when RegExp and RegExp result are fast wherever possible.

As shown below, this CL improves the performance of calling S.p.matchAll and
iterating over matches.

Before:

StringMatchAllBuiltinRegExpIteratorCreation-Strings(Score): 5002
StringMatchAllBuiltinStringIteratorCreation-Strings(Score): 13798
StringMatchAllBuiltinString-Strings(Score): 197
StringMatchAllManualString-Strings(Score): 454
StringMatchAllBuiltinRegExp-Strings(Score): 193
StringMatchAllManualRegExp-Strings(Score): 453
StringMatchAllBuiltinZeroWidth-Strings(Score): 97.2
StringMatchAllBuiltinZeroWidthUnicode-Strings(Score): 95.9

After:

StringMatchAllBuiltinRegExpIteratorCreation-Strings(Score): 15437
StringMatchAllBuiltinStringIteratorCreation-Strings(Score): 16708
StringMatchAllBuiltinString-Strings(Score): 392
StringMatchAllManualString-Strings(Score): 452
StringMatchAllBuiltinRegExp-Strings(Score): 394
StringMatchAllManualRegExp-Strings(Score): 484
StringMatchAllBuiltinZeroWidth-Strings(Score): 409
StringMatchAllBuiltinZeroWidthUnicode-Strings(Score): 413

Bug: v8:6890
Change-Id: I6fcc1003a471314cf412aac456d42286b2926810
Reviewed-on: https://chromium-review.googlesource.com/1005400
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52581}
[modify] https://crrev.com/ae4529f9e1e635d49364c9e62801d067dc806a95/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/ae4529f9e1e635d49364c9e62801d067dc806a95/src/builtins/builtins-regexp-gen.h
[modify] https://crrev.com/ae4529f9e1e635d49364c9e62801d067dc806a95/src/builtins/builtins-string-gen.cc

Comment 9 by bugdroid1@chromium.org, May 8 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/cc399a0dc4627de193700a34c75717a653bb3107

commit cc399a0dc4627de193700a34c75717a653bb3107
Author: jgruber <jgruber@chromium.org>
Date: Tue May 08 08:27:36 2018

Stage String.p.matchAll

Bug: v8:6890
Change-Id: I4002326cb79165ce6edb79a943d66de156b90116
Reviewed-on: https://chromium-review.googlesource.com/1046053
Reviewed-by: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53052}
[modify] https://crrev.com/cc399a0dc4627de193700a34c75717a653bb3107/src/flag-definitions.h

Comment 10 by bugdroid1@chromium.org, Jun 6 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8789d804ec392c2dd4d9c95340ddeaaab22c8d2e

commit 8789d804ec392c2dd4d9c95340ddeaaab22c8d2e
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Wed Jun 06 01:07:16 2018

[esnext] Update String.p.matchAll as per spec changes

As per (https://github.com/tc39/proposal-string-matchall/pull/35), the
call to IsRegExp after CreateRegExp was removed and additional
checking was replaced by an Assert.

Updates to Test262 has been submitted:
https://github.com/tc39/test262/pull/1587

Bug: v8:6890
Change-Id: I942b6846bb46cf85b1ea5566f9c19de7d2dbf03e
Reviewed-on: https://chromium-review.googlesource.com/1086419
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Cr-Commit-Position: refs/heads/master@{#53539}
[modify] https://crrev.com/8789d804ec392c2dd4d9c95340ddeaaab22c8d2e/src/builtins/builtins-regexp-gen.cc

Comment 11 by bugdroid1@chromium.org, Jun 21 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/7977035f808e442d03fbf979b75292e3ba65c3b1

commit 7977035f808e442d03fbf979b75292e3ba65c3b1
Author: Mathias Bynens <mathias@chromium.org>
Date: Thu Jun 21 15:03:56 2018

Roll Test262

Bug: v8:6890,  v8:7825 , v8:7834, v8:7874
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: Idc5e532977e2309de55a27ea6513301389b3a525
Reviewed-on: https://chromium-review.googlesource.com/1110120
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Commit-Queue: Mathias Bynens <mathias@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53934}
[modify] https://crrev.com/7977035f808e442d03fbf979b75292e3ba65c3b1/DEPS
[modify] https://crrev.com/7977035f808e442d03fbf979b75292e3ba65c3b1/test/test262/test262.status
[modify] https://crrev.com/7977035f808e442d03fbf979b75292e3ba65c3b1/test/test262/testcfg.py

Comment 12 by mathias@chromium.org, Oct 3

The Test262 tests have been updated as per recent changes in the proposal: https://github.com/tc39/test262/pull/1788

Comment 13 by jgruber@chromium.org, Oct 8

NextAction: 2018-10-22
Setting reminder to check back in 2 weeks.

Comment 14 by monor...@bugs.chromium.org, Oct 22

The NextAction date has arrived: 2018-10-22

Comment 15 by jgruber@chromium.org, Oct 22

NextAction: 2018-11-05
Bumping another 2 weeks, Peter this is still on your radar right?

Comment 16 by peter.wm...@gmail.com, Oct 23

Yup still on my radar.

Comment 17 by monor...@bugs.chromium.org, Nov 5

The NextAction date has arrived: 2018-11-05

Comment 18 by peter.wm...@gmail.com, Nov 5

I have the slow path updated and passing all matchall test262 tests.
I was planning on landing this first, then follow up with the fast path.

Comment 19 by peter.wm...@gmail.com, Nov 6

NextAction: 2018-11-30
After double checking the latest updates on the spec repo, their appears to be some
loose ends that need to be tied up: 

https://github.com/tc39/proposal-string-matchall/issues/39#issuecomment-436132188

Jordan Harband (champion) -
    A delegate requested that we ask, instead of notify, the committee - so we can’t
    move forward until after the November meeting either way.

    If tc39/ecma262#1318 or a similar change seems like it could proceed, then
    option 2 would be acceptable - we’ll have to see how the November meeting plays out.

I will continue with my current CL get us passing with the current test262 matchAll tests.
And then follow up after TC39's Nov 27-29th meeting.

Comment 20 by bugdroid1@chromium.org, Nov 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/0f249dd8159b186f88f193c35ee832719bee3f48

commit 0f249dd8159b186f88f193c35ee832719bee3f48
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Wed Nov 14 08:54:02 2018

[builtins] Adjust String.prototype.matchAll as per spec changes (https://github.com/tc39/proposal-string-matchall/pull/38)

- Removes IsRegExp check and special handling when false
- Removes MatchAllIterator
- Extracts previously inlined CreateRegExpStringIterator
- Update comments to match spec text and numbering

Bug: v8:6890
Change-Id: Ie81757a499acc77910f029835fb042e70d86d83d
Reviewed-on: https://chromium-review.googlesource.com/c/1317830
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57488}
[modify] https://crrev.com/0f249dd8159b186f88f193c35ee832719bee3f48/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/0f249dd8159b186f88f193c35ee832719bee3f48/src/builtins/builtins-regexp-gen.h
[modify] https://crrev.com/0f249dd8159b186f88f193c35ee832719bee3f48/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/0f249dd8159b186f88f193c35ee832719bee3f48/test/test262/test262.status

Comment 21 by mathias@chromium.org, Nov 29

At today’s TC39 meeting, we got consensus to remove the fallback behavior. The change boils down to: https://github.com/tc39/proposal-string-matchall/pull/41

The observable delta here is that `matchAll` now throws after `delete RegExp.prototype[Symbol.matchAll]`.

Once we update our implementation, we can consider shipping the feature!

Comment 22 by monor...@bugs.chromium.org, Nov 30

The NextAction date has arrived: 2018-11-30

Comment 23 by peter.wm...@gmail.com, Dec 3

NextAction: 2018-12-10
Cool, I'm a bit busy this coming week, but I should have be able to make this small update plus an update to test262 tests by next Monday.

Comment 24 by gsat...@chromium.org, Dec 3

Great, thanks for the update Peter. FYI, the feature freeze for Chrome 73 is Jan 11.

Comment 25 by peter.wm...@gmail.com, Dec 10

Just a quick update, as promised...
- CL in-flight: https://chromium-review.googlesource.com/c/v8/v8/+/1369091
- Test262 PR in-flight (champion already LGTM): https://github.com/tc39/test262/pull/1990

Both of are waiting for the spec update PR to be merged: https://github.com/tc39/proposal-string-matchall/pull/41

Comment 26 by monor...@bugs.chromium.org, Dec 10

The NextAction date has arrived: 2018-12-10

Comment 27 by peter.wm...@gmail.com, Dec 11

Blockedon: 8574

Comment 28 by bugdroid1@chromium.org, Dec 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/29a970a27319a7e1c193c026a817f037bb4ef71f

commit 29a970a27319a7e1c193c026a817f037bb4ef71f
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Tue Dec 11 23:05:54 2018

[esnext] Update String.p.matchAll as per spec changes

As per (https://github.com/tc39/proposal-string-matchall/pull/41), String.p.matchAll's fallback was removed.
Additionally, removed a IsNullOrUndefined check that was already covered by MaybeCallFunctionAtSymbol.
Updates to Test262 has been submitted: https://github.com/tc39/test262/pull/1990

Bug: v8:6890
Change-Id: I246cbbcb4641ebded704c5f772809f182deaa30e
Reviewed-on: https://chromium-review.googlesource.com/c/1369091
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58172}
[modify] https://crrev.com/29a970a27319a7e1c193c026a817f037bb4ef71f/src/builtins/builtins-string-gen.cc
[add] https://crrev.com/29a970a27319a7e1c193c026a817f037bb4ef71f/test/mjsunit/harmony/string-matchAll-deleted-matchAll.js
[modify] https://crrev.com/29a970a27319a7e1c193c026a817f037bb4ef71f/test/test262/test262.status

Comment 29 by peter.wm...@gmail.com, Dec 13

Sathya and Mathias,
I think we're good on the test262 tests and the implementation
In V8.  What's the next stop on the journey? :)

Comment 30 by mathias@chromium.org, Dec 13

Peter, do you want to prepare an Intent to Ship email? Example: https://groups.google.com/a/chromium.org/d/msg/blink-dev/-pNjKEJ9YBQ/EvxoBANGAwAJ

Comment 31 by jgruber@chromium.org, Dec 13

Created the chromium feature page, Peter I shared the auto-generated email template with you:

https://docs.google.com/document/d/1SSzs0LQAU1RGbQZXdxNVt335Vj5InH6plSm-7E7iqi8/edit?usp=sharing

Seems like my first submission was partially swallowed so some fields are empty in the template :( I'll update the feature page once it's up.

Comment 32 by jgruber@chromium.org, Dec 13

Components: Regexp

Comment 33 by gsat...@chromium.org, Dec 13

Peter, please use a format similar to the existing V8 specific I2S, not the chromestatus generated one: 
https://groups.google.com/a/chromium.org/d/msg/blink-dev/Lawn0wM156I/vFjgt_9WCAAJ

Most of it is the same but there are some differences (requesting approval, tests, etc). Let me know if you'd like me to take a look before sending it out.

Comment 34 by peter.wm...@gmail.com, Dec 13

Thanks all for the pointers on filling out the Intent to Ship!
I have updated Jakob's template.

Sathya, I'll take you up on your offer.
Would you please take a look and let me know if it looks good to you (and feel free to edit at will)?
https://docs.google.com/document/d/1SSzs0LQAU1RGbQZXdxNVt335Vj5InH6plSm-7E7iqi8/edit

Thanks again!

Comment 35 by gsat...@chromium.org, Dec 13

Added a note about passing all the tests. LGTM to send out, thanks! Please send it to v8-users, v8-dev and blink-dev.

Comment 36 by peter.wm...@gmail.com, Dec 13

Thanks Sathya. Bombs away! Emails sent to v8-users, v8-dev and blink-dev.
I guess this is my first post to blink-dev, so I think it's stuck in "moderation".

Comment 37 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/76cb4fe6263dcfd6f97137dbb4905283b75d3ce2

commit 76cb4fe6263dcfd6f97137dbb4905283b75d3ce2
Author: peterwmwong <peter.wm.wong@gmail.com>
Date: Fri Dec 14 18:32:51 2018

[esnext] Ship String.p.matchAll/RegExp.p.[@@matchAll]

Enable --harmony-string-matchall by default.

String.prototype.matchAll behaves similarly to
String.prototype.match, but returns a full regexp
result object for each match in a global or sticky
regexp. This offers a simple way to iterate over
matches when access to e.g. capture groups is
needed.

const string = 'a b c';
const regex = /[ac]/g;
for (const match of string.matchAll(regex)) {
  console.log(`${match[0]} at ${match.index}`);
}
// a at 0
// c at 4

More information can be found here:
https://github.com/tc39/proposal-string-matchall

Drive-by: Update debug evaluate side effect
expectations to handle String.p.matchAll and
RegExp.p[@@matchAll]

Bug: v8:6890
Change-Id: Ie3e712af66689936b7d2a15df705b792ccf06bd3
Reviewed-on: https://chromium-review.googlesource.com/c/1377774
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Cr-Commit-Position: refs/heads/master@{#58250}
[modify] https://crrev.com/76cb4fe6263dcfd6f97137dbb4905283b75d3ce2/src/debug/debug-evaluate.cc
[modify] https://crrev.com/76cb4fe6263dcfd6f97137dbb4905283b75d3ce2/src/flag-definitions.h
[modify] https://crrev.com/76cb4fe6263dcfd6f97137dbb4905283b75d3ce2/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-builtins.js

Sign in to add a comment