(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?
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
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
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
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
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
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
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
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
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
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 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.
(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 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 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.
2019-06-13 00:04 PDT, Alexey Shvayka
2019-06-13 01:27 PDT, EWS Watchlist
2019-06-13 01:36 PDT, EWS Watchlist
2019-06-13 01:47 PDT, EWS Watchlist
2019-06-13 02:03 PDT, EWS Watchlist
2019-06-13 02:15 PDT, EWS Watchlist
2019-06-13 08:50 PDT, Alexey Shvayka
2019-06-13 10:01 PDT, EWS Watchlist
2019-06-13 10:20 PDT, EWS Watchlist
2019-06-13 10:46 PDT, EWS Watchlist
2019-06-13 10:59 PDT, EWS Watchlist
2019-06-13 11:09 PDT, EWS Watchlist
2019-06-13 18:10 PDT, Alexey Shvayka
2019-06-18 12:24 PDT, Alexey Shvayka