Bug 186694 – [ESNExt] String.prototype.matchAll

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Leo Balter]

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

Already on Stage 3

Using the respective Test262 features:

String.prototype.matchAll
Symbol.matchAll

[Alexey Shvayka]

Created attachment 372025 [details]
Patch

[Alexey Shvayka]

(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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[jsc-armv7 EWS]

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

[Alexey Shvayka]

Created attachment 372055 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

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.

[Alexey Shvayka]

Created attachment 372089 [details]
Patch

[Alexey Shvayka]

(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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Alexey Shvayka]

Created attachment 372361 [details]
Patch

Rename @globalPrivate @constructors and C++ variables that hold them.

[Yusuke Suzuki]

Comment on attachment 372361 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 372361 [details]
Patch

Clearing flags on attachment: 372361

Committed r246567: <https://trac.webkit.org/changeset/246567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51865538>