Bug 81459 – Strength reduction, RegExp.exec -> RegExp.test

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

[Gavin Barraclough]

RegExp.prototype.exec & RegExp.prototype.test can both be used to test a regular expression for a match against a string - however exec is more expensive, since it allocates a matches array object.  In cases where the result is consumed in a boolean context the allocation of the matches array can be trivially elided.

For example:
    function f()
    {
        for (i =0; i < 10000000; ++i)
            if(!/a/.exec("a"))
                err = true;
    }

This is a 2.5x speedup on this example microbenchmark loop.

In a more advanced for of this optimization, we may be able to avoid allocating the array where access to the array can be observed.

[Gavin Barraclough]

Created attachment 132473 [details]
Fix

[WebKit Review Bot]

Attachment 132473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:64:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:65:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:67:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:68:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:70:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:72:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:73:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:78:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:79:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:80:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:81:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:82:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:83:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:84:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:87:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:90:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:91:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:94:  DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpObject.h:70:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpObject.h:71:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 21 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Fixed in r111129

[SravanKumar S(:sravan)]

Hi Gavin,

The code changes are still resulting in windows build break(as of 111160). The error is same as mentioned by win build bot. Can you please fix it ASAP. I am stalled by this.

[Jessie Berlin]

3>JSBase.obj : warning LNK4217: locally defined symbol ?staticData@WTFThreadData@WTF@@0PAV?$ThreadSpecific@VWTFThreadData@WTF@@@2@A (private: static class WTF::ThreadSpecific<class WTF::WTFThreadData> * WTF::WTFThreadData::staticData) imported in function "public: __thiscall JSC::APIEntryShim::~APIEntryShim(void)" (??1APIEntryShim@JSC@@QAE@XZ)
3>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: static class JSC::JSFunction * __cdecl JSC::JSFunction::create(class JSC::ExecState *,class JSC::JSGlobalObject *,int,class JSC::Identifier const &,__int64 (__fastcall*)(class JSC::ExecState *),__int64 (__fastcall*)(class JSC::ExecState *))" (?create@JSFunction@JSC@@SAPAV12@PAVExecState@2@PAVJSGlobalObject@2@HABVIdentifier@2@P6I_J0@Z3@Z)
3>C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\bin\JavaScriptCore.dll : fatal error LNK1120: 1 unresolved externals
3>Build log was saved at "file://C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\obj\JavaScriptCore\BuildLog.htm"
3>JavaScriptCore - 2 error(s), 16 warning(s)

http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/47242/steps/compile-webkit/logs/stdio

[Jessie Berlin]

(In reply to comment #5)
> 3>JSBase.obj : warning LNK4217: locally defined symbol ?staticData@WTFThreadData@WTF@@0PAV?$ThreadSpecific@VWTFThreadData@WTF@@@2@A (private: static class WTF::ThreadSpecific<class WTF::WTFThreadData> * WTF::WTFThreadData::staticData) imported in function "public: __thiscall JSC::APIEntryShim::~APIEntryShim(void)" (??1APIEntryShim@JSC@@QAE@XZ)
> 3>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: static class JSC::JSFunction * __cdecl JSC::JSFunction::create(class JSC::ExecState *,class JSC::JSGlobalObject *,int,class JSC::Identifier const &,__int64 (__fastcall*)(class JSC::ExecState *),__int64 (__fastcall*)(class JSC::ExecState *))" (?create@JSFunction@JSC@@SAPAV12@PAVExecState@2@PAVJSGlobalObject@2@HABVIdentifier@2@P6I_J0@Z3@Z)
> 3>C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\bin\JavaScriptCore.dll : fatal error LNK1120: 1 unresolved externals
> 3>Build log was saved at "file://C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\obj\JavaScriptCore\BuildLog.htm"
> 3>JavaScriptCore - 2 error(s), 16 warning(s)
> 
> http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/47242/steps/compile-webkit/logs/stdio

Fixed in http://trac.webkit.org/changeset/111188

[Gavin Barraclough]

Ooops!- thanks Jessie!

[SravanKumar S(:sravan)]

Hi, The issue is still present when building WebKit project, it is fixed when building JavaScriptCore though.

5>   Creating library C:\cygwin\home\ssandela\new_webkit\WebKit\WebKitBuild\Debug\lib\WebKit.lib and object C:\cygwin\home\ssandela\new_webkit\WebKit\WebKitBuild\Debug\lib\WebKit.exp
5>WebCore.lib(JSBindingsAllInOne.obj) : error LNK2001: unresolved external symbol "public: static class JSC::JSFunction * __cdecl JSC::JSFunction::create(class JSC::ExecState *,class JSC::JSGlobalObject *,int,class JSC::Identifier const &,__int64 (__fastcall*)(class JSC::ExecState *),enum JSC::Intrinsic,__int64 (__fastcall*)(class JSC::ExecState *))" (?create@JSFunction@JSC@@SAPAV12@PAVExecState@2@PAVJSGlobalObject@2@HABVIdentifier@2@P6I_J0@ZW4Intrinsic@2@3@Z)
5>C:\cygwin\home\ssandela\new_webkit\WebKit\WebKitBuild\Debug\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals

[Jessie Berlin]

(In reply to comment #8)
> Hi, The issue is still present when building WebKit project, it is fixed when building JavaScriptCore though.
> 
> 5>   Creating library C:\cygwin\home\ssandela\new_webkit\WebKit\WebKitBuild\Debug\lib\WebKit.lib and object C:\cygwin\home\ssandela\new_webkit\WebKit\WebKitBuild\Debug\lib\WebKit.exp
> 5>WebCore.lib(JSBindingsAllInOne.obj) : error LNK2001: unresolved external symbol "public: static class JSC::JSFunction * __cdecl JSC::JSFunction::create(class JSC::ExecState *,class JSC::JSGlobalObject *,int,class JSC::Identifier const &,__int64 (__fastcall*)(class JSC::ExecState *),enum JSC::Intrinsic,__int64 (__fastcall*)(class JSC::ExecState *))" (?create@JSFunction@JSC@@SAPAV12@PAVExecState@2@PAVJSGlobalObject@2@HABVIdentifier@2@P6I_J0@ZW4Intrinsic@2@3@Z)
> 5>C:\cygwin\home\ssandela\new_webkit\WebKit\WebKitBuild\Debug\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals

This was fixed in http://trac.webkit.org/changeset/111197

[Filip Pizlo]

Comment on attachment 132473 [details]
Fix

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

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:638
> +    case RegExpExec:
> +    case RegExpTest:
> +        forNode(node.child1()).filter(PredictCell);
> +        forNode(node.child2()).filter(PredictCell);
> +        forNode(nodeIndex).makeTop();
> +        break;
> +            

This AI rule is wrong because it fails to call clobberStructures(), or clobberWorld() in the modern DFG.  This bug has managed to survive until now!

The implication is that RegExpExec/RegExpTest could clobber the world if the second argument is an object with a toString method, but AI would incorrectly assume that all of its structure proofs are still intact.  Because of this logic, when we wrote clobberize() we also failed to make RegExpExec/RegExpTest clobber the world there.  That means we will CSE and LICM around these clobbering operations, too.