Bug 186694 - [ESNExt] String.prototype.matchAll
Summary: [ESNExt] String.prototype.matchAll
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-15 14:20 PDT by Leo Balter
Modified: 2019-06-18 14:22 PDT (History)
9 users (show)

See Also:


Attachments
Patch (20.41 KB, patch)
2019-06-13 00:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.18 MB, application/zip)
2019-06-13 01:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.71 MB, application/zip)
2019-06-13 01:36 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews215 for win-future (13.51 MB, application/zip)
2019-06-13 01:47 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.98 MB, application/zip)
2019-06-13 02:03 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.60 MB, application/zip)
2019-06-13 02:15 PDT, EWS Watchlist
no flags Details
Patch (23.90 KB, patch)
2019-06-13 08:50 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.31 MB, application/zip)
2019-06-13 10:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.69 MB, application/zip)
2019-06-13 10:20 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.03 MB, application/zip)
2019-06-13 10:46 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.66 MB, application/zip)
2019-06-13 10:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews213 for win-future (13.74 MB, application/zip)
2019-06-13 11:09 PDT, EWS Watchlist
no flags Details
Patch (31.78 KB, patch)
2019-06-13 18:10 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (43.18 KB, patch)
2019-06-18 12:24 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Balter 2018-06-15 14:20:27 PDT
https://github.com/tc39/proposal-string-matchall

Already on Stage 3

Using the respective Test262 features:

String.prototype.matchAll
Symbol.matchAll
Comment 1 Alexey Shvayka 2019-06-13 00:04:20 PDT
Created attachment 372025 [details]
Patch
Comment 2 Alexey Shvayka 2019-06-13 00:20:37 PDT
(In reply to Alexey Shvayka from comment #1)
> Created attachment 372025 [details]
> Patch

This change is spec-perfect, but I am still working on fast path for `match`.
I would really appreciate a hint on why changes in RegExpStringIteratorPrototype.js aren't recompiled properly most of the time.
Am I adding new sources correctly?
Comment 3 EWS Watchlist 2019-06-13 01:27:05 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12462184

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 4 EWS Watchlist 2019-06-13 01:27:06 PDT
Created attachment 372031 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 5 EWS Watchlist 2019-06-13 01:36:24 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12462194

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 6 EWS Watchlist 2019-06-13 01:36:25 PDT
Created attachment 372032 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 7 EWS Watchlist 2019-06-13 01:46:59 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12462205

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 8 EWS Watchlist 2019-06-13 01:47:01 PDT
Created attachment 372033 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 9 EWS Watchlist 2019-06-13 02:03:35 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12462235

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 10 EWS Watchlist 2019-06-13 02:03:36 PDT
Created attachment 372035 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 11 EWS Watchlist 2019-06-13 02:06:27 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12462190

New failing tests:
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-ftl
apiTests
Comment 12 EWS Watchlist 2019-06-13 02:15:04 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12462332

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 13 EWS Watchlist 2019-06-13 02:15:06 PDT
Created attachment 372036 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5
Comment 14 jsc-armv7 EWS 2019-06-13 02:48:47 PDT
Comment on attachment 372025 [details]
Patch

Attachment 372025 [details] did not pass jsc-armv7-ews (jsc-only):
Output: https://webkit-queues.webkit.org/results/12462350

New failing tests:
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-llint
apiTests
Comment 15 Alexey Shvayka 2019-06-13 08:50:13 PDT
Created attachment 372055 [details]
Patch
Comment 16 EWS Watchlist 2019-06-13 10:01:37 PDT
Comment on attachment 372055 [details]
Patch

Attachment 372055 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12465778

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 17 EWS Watchlist 2019-06-13 10:01:39 PDT
Created attachment 372062 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 18 EWS Watchlist 2019-06-13 10:20:20 PDT
Comment on attachment 372055 [details]
Patch

Attachment 372055 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12465841

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 19 EWS Watchlist 2019-06-13 10:20:21 PDT
Created attachment 372064 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 20 EWS Watchlist 2019-06-13 10:46:27 PDT
Comment on attachment 372055 [details]
Patch

Attachment 372055 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12465865

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 21 EWS Watchlist 2019-06-13 10:46:29 PDT
Created attachment 372067 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 22 EWS Watchlist 2019-06-13 10:51:45 PDT
Comment on attachment 372055 [details]
Patch

Attachment 372055 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12465840

New failing tests:
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-ftl
apiTests
Comment 23 EWS Watchlist 2019-06-13 10:59:42 PDT
Comment on attachment 372055 [details]
Patch

Attachment 372055 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12465889

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 24 EWS Watchlist 2019-06-13 10:59:44 PDT
Created attachment 372068 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5
Comment 25 EWS Watchlist 2019-06-13 11:09:12 PDT
Comment on attachment 372055 [details]
Patch

Attachment 372055 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12465989

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 26 EWS Watchlist 2019-06-13 11:09:16 PDT
Created attachment 372069 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 27 Yusuke Suzuki 2019-06-13 11:16:01 PDT
Comment on attachment 372055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372055&action=review

The logic looks nice.

> Source/JavaScriptCore/CMakeLists.txt:934
> +    runtime/RegExpStringIteratorPrototype.h

Let’s add these files (added h, cpp, and generated code from js file etc. maybe js file too.) to Xcode project file too?
At that time, be careful that Xcode should not build the added cpp files directly. We are using unified builds.
So we need to set “target” configuration in Xcode property.
See the similar files and align these files to them.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:28
> +function RegExpStringIteratorConstructor(R, S, global, fullUnicode)

Rename this to createRegExpStringIterator.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:163
> +    let R = this;

I understand that these names come from the spec. But we use descriptive name in our source code.
Can you replace R to regexp, S to string etc. in your patch?

> Source/JavaScriptCore/runtime/RegExpStringIteratorPrototype.h:34
> +    typedef JSNonFinalObject Base;

Use “using” in newly added code.

> Source/JavaScriptCore/runtime/RegExpStringIteratorPrototype.h:39
> +        auto structure = Structure::create(vm, globalObject, iteratorPrototype, TypeInfo(ObjectType, StructureFlags), info());

Create createStructure function, call it in JSGlobalObject side and pass the structure to this function. I think this is the convention in JSC.
Comment 28 Alexey Shvayka 2019-06-13 18:10:14 PDT
Created attachment 372089 [details]
Patch
Comment 29 Alexey Shvayka 2019-06-13 18:12:14 PDT
(In reply to Yusuke Suzuki from comment #27)
> Comment on attachment 372055 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372055&action=review
> 
> The logic looks nice.
> 
> > Source/JavaScriptCore/CMakeLists.txt:934
> > +    runtime/RegExpStringIteratorPrototype.h
> 
> Let’s add these files (added h, cpp, and generated code from js file etc.
> maybe js file too.) to Xcode project file too?
> At that time, be careful that Xcode should not build the added cpp files
> directly. We are using unified builds.
> So we need to set “target” configuration in Xcode property.
> See the similar files and align these files to them.

Thank you for detailed tips, I've fixed the build via Xcode UI.

> 
> > Source/JavaScriptCore/builtins/RegExpPrototype.js:28
> > +function RegExpStringIteratorConstructor(R, S, global, fullUnicode)
> 
> Rename this to createRegExpStringIterator.
>

Using `new` with `create*` function looks kinda weird.
Idiomatic JS is `new` with title-cased constructor, just like we already have with AsyncFromSyncIterator:

```
return new @AsyncFromSyncIteratorConstructor(syncIterator, nextMethod);
```

AsyncFromSyncIterator does indeed expose `createAsyncFromSyncIterator` helper for BytecodeGenerator,
but I not sure it is something we need for RegExpStringIterator.

===

On performance: we can't use any of already defined fast RegExp methods.
We can avoid extra sets and property lookups in two places:
1. After @speciesConstructor in RegExp.prototype[@@matchAll]
2. After last @getByIdDirectPrivate in %RegExpStringIteratorPrototype%.next.

Given that we will have to call @hasObservableSideEffects in `next` every time, does it worth it?
Comment 30 Yusuke Suzuki 2019-06-16 17:35:48 PDT
Comment on attachment 372055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372055&action=review

>>> Source/JavaScriptCore/builtins/RegExpPrototype.js:28
>>> +function RegExpStringIteratorConstructor(R, S, global, fullUnicode)
>> 
>> Rename this to createRegExpStringIterator.
> 
> Using `new` with `create*` function looks kinda weird.
> Idiomatic JS is `new` with title-cased constructor, just like we already have with AsyncFromSyncIterator:
> 
> ```
> return new @AsyncFromSyncIteratorConstructor(syncIterator, nextMethod);
> ```
> 
> AsyncFromSyncIterator does indeed expose `createAsyncFromSyncIterator` helper for BytecodeGenerator,
> but I not sure it is something we need for RegExpStringIterator.
> 
> ===
> 
> On performance: we can't use any of already defined fast RegExp methods.
> We can avoid extra sets and property lookups in two places:
> 1. After @speciesConstructor in RegExp.prototype[@@matchAll]
> 2. After last @getByIdDirectPrivate in %RegExpStringIteratorPrototype%.next.
> 
> Given that we will have to call @hasObservableSideEffects in `next` every time, does it worth it?

But instead, you introduced "RegExpStringIteratorConstructor" variable in JSGlobalObject.cpp. In WebKit, variables start with lower case.
If you want to use this, can you do the following things too?

1. Rename RegExpStringIteratorConstructor to RegExpStringIterator
2. Fix the script not to expose RegExpStringIterator as a variable. Instead, we should expose regExpStringIterator
3. And fix all the existing ones too.

For performance perspective, I think this is OK for the first implementation.
Comment 31 Yusuke Suzuki 2019-06-16 17:46:38 PDT
Comment on attachment 372055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372055&action=review

>>>> Source/JavaScriptCore/builtins/RegExpPrototype.js:28
>>>> +function RegExpStringIteratorConstructor(R, S, global, fullUnicode)
>>> 
>>> Rename this to createRegExpStringIterator.
>> 
>> Using `new` with `create*` function looks kinda weird.
>> Idiomatic JS is `new` with title-cased constructor, just like we already have with AsyncFromSyncIterator:
>> 
>> ```
>> return new @AsyncFromSyncIteratorConstructor(syncIterator, nextMethod);
>> ```
>> 
>> AsyncFromSyncIterator does indeed expose `createAsyncFromSyncIterator` helper for BytecodeGenerator,
>> but I not sure it is something we need for RegExpStringIterator.
>> 
>> ===
>> 
>> On performance: we can't use any of already defined fast RegExp methods.
>> We can avoid extra sets and property lookups in two places:
>> 1. After @speciesConstructor in RegExp.prototype[@@matchAll]
>> 2. After last @getByIdDirectPrivate in %RegExpStringIteratorPrototype%.next.
>> 
>> Given that we will have to call @hasObservableSideEffects in `next` every time, does it worth it?
> 
> But instead, you introduced "RegExpStringIteratorConstructor" variable in JSGlobalObject.cpp. In WebKit, variables start with lower case.
> If you want to use this, can you do the following things too?
> 
> 1. Rename RegExpStringIteratorConstructor to RegExpStringIterator
> 2. Fix the script not to expose RegExpStringIterator as a variable. Instead, we should expose regExpStringIterator
> 3. And fix all the existing ones too.
> 
> For performance perspective, I think this is OK for the first implementation.

Hmm. After considering about my proposal above, I think above proposal is not perfect yet :(
I think the better one should be the followings.

1. Function name in JS should be RegExpStringIterator. Remove "Constructor".
2. But the variable name should have "Constructor" in C++
3. We do not use title-case variable name.

So, I think the better fix is,

1. If @constructor annotation is attached, we only allowed title-case name in builtin JS function (ensuring it in the builtin script processor). I think we can also assert that all non @constructor function starts with lower-case character.
2. If @constructor annotation is attached, we convert title-case name to usual WebKit variable name for C++ code, and add "Constructor" name too for C++ in the builtin script processor.
3. And apply the above changes to the all existing ones.

So, the code looks like,

@globalPrivate
@constructor
function RegExpStringIterator(......)
{
    ...
}

And, C++ code looks like,

regExpStringIteratorConstructor ...

I think this is better.
Comment 32 Alexey Shvayka 2019-06-18 12:24:24 PDT
Created attachment 372361 [details]
Patch

Rename @globalPrivate @constructors and C++ variables that hold them.
Comment 33 Yusuke Suzuki 2019-06-18 13:11:04 PDT
Comment on attachment 372361 [details]
Patch

r=me
Comment 34 WebKit Commit Bot 2019-06-18 14:21:55 PDT
Comment on attachment 372361 [details]
Patch

Clearing flags on attachment: 372361

Committed r246567: <https://trac.webkit.org/changeset/246567>
Comment 35 WebKit Commit Bot 2019-06-18 14:21:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Radar WebKit Bug Importer 2019-06-18 14:22:26 PDT
<rdar://problem/51865538>