Bug 81459 - Strength reduction, RegExp.exec -> RegExp.test
Summary: Strength reduction, RegExp.exec -> RegExp.test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-17 17:15 PDT by Gavin Barraclough
Modified: 2016-03-05 15:07 PST (History)
3 users (show)

See Also:


Attachments
Fix (33.36 KB, patch)
2012-03-17 17:37 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2012-03-17 17:15:47 PDT
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.
Comment 1 Gavin Barraclough 2012-03-17 17:37:44 PDT
Created attachment 132473 [details]
Fix
Comment 2 WebKit Review Bot 2012-03-17 17:51:08 PDT
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.
Comment 3 Gavin Barraclough 2012-03-17 18:08:43 PDT
Fixed in r111129
Comment 4 SravanKumar S(:sravan) 2012-03-19 06:58:35 PDT
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.
Comment 5 Jessie Berlin 2012-03-19 08:55:20 PDT
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
Comment 6 Jessie Berlin 2012-03-19 09:19:34 PDT
(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
Comment 7 Gavin Barraclough 2012-03-19 11:28:22 PDT
Ooops!- thanks Jessie!
Comment 8 SravanKumar S(:sravan) 2012-03-19 20:54:33 PDT
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
Comment 9 Jessie Berlin 2012-03-20 10:13:32 PDT
(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
Comment 10 Filip Pizlo 2016-03-05 15:07:39 PST
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.