Bug 122704 – Baseline JIT should use the DFG's PutById IC

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 214068 [details]
it begins

[Filip Pizlo]

Created attachment 214110 [details]
slowly but surely...

[Oliver Hunt]

Comment on attachment 214110 [details]
slowly but surely...

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

> Source/JavaScriptCore/jit/JIT.h:260
> +            : m_type(PutById)
> +            , bytecodeIndex(bytecodeIndex)
> +            , structureToCompare(structureToCompare)
> +            , strucureCheck(structureCheck)
> +            , propertyStorageLoad(propertyStorageLoad)
> +#if USE(JSVALUE64)
> +            , putDisplacementLabel(displacementLabel)
> +#else
> +            , putDisplacementLabel1(displacementLabel1)
> +            , putDisplacementLabel2(displacementLabel2)
> +#endif
> +            , done(done)

indent or the style bot will complain

> Source/JavaScriptCore/jit/JIT.h:268
> -            getColdPathBegin = coldPathBegin;
> +            this->coldPathBegin = coldPathBegin;

Can we just rename the members to be m_... ?  (I'm okay with not doing in this patch if there's a pile of usages that make it awful to change)

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:-610
> -    BEGIN_UNINTERRUPTED_SEQUENCE(sequencePutById);

Do we still need BEGIN/END_UNINTERRUPTED_SEQUENCE ?  The branch compactor uses (used to?) use these to indicate branches that could not be rewritten.  If it has better handling now, yay!

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 214110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214110&action=review
> 
> > Source/JavaScriptCore/jit/JIT.h:260
> > +            : m_type(PutById)
> > +            , bytecodeIndex(bytecodeIndex)
> > +            , structureToCompare(structureToCompare)
> > +            , strucureCheck(structureCheck)
> > +            , propertyStorageLoad(propertyStorageLoad)
> > +#if USE(JSVALUE64)
> > +            , putDisplacementLabel(displacementLabel)
> > +#else
> > +            , putDisplacementLabel1(displacementLabel1)
> > +            , putDisplacementLabel2(displacementLabel2)
> > +#endif
> > +            , done(done)
> 
> indent or the style bot will complain

No it won't ... this is already indented the same way as the other constructor overloads.

> 
> > Source/JavaScriptCore/jit/JIT.h:268
> > -            getColdPathBegin = coldPathBegin;
> > +            this->coldPathBegin = coldPathBegin;
> 
> Can we just rename the members to be m_... ?  (I'm okay with not doing in this patch if there's a pile of usages that make it awful to change)

I don't think it would be a good idea to make this change anytime soon.  This patch is an incremental step towards getting rid of the baseline JIT's IC logic entirely.  So, doing refactorings to "clean up" that logic in the middle of getting rid of it sounds like it might be a waste of time.

> 
> > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:-610
> > -    BEGIN_UNINTERRUPTED_SEQUENCE(sequencePutById);
> 
> Do we still need BEGIN/END_UNINTERRUPTED_SEQUENCE ?  The branch compactor uses (used to?) use these to indicate branches that could not be rewritten.  If it has better handling now, yay!

The DFG JIT doesn't use it, so I'm assuming not.

[Filip Pizlo]

Created attachment 214115 [details]
small steps

I should just have this done by now but I'm feeling sooooper lazy.

[Filip Pizlo]

Created attachment 214120 [details]
almost works

Need to change the JITOperations for PutById to take EncodedJSValue for the base rather than JSCell*.

Also need to kill the old Baseline JIT put_by_id IC code, since that's now unused.

[Filip Pizlo]

Created attachment 214124 [details]
the patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/PutByIdStatus.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/JITPropertyAccess.cpp', u'Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/jit/Repatch.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:612:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:553:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 14 files


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

[Filip Pizlo]

(In reply to comment #8)
> Attachment 214124 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/PutByIdStatus.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/JITPropertyAccess.cpp', u'Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/jit/Repatch.cpp']" exit_code: 1
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:612:  Missing space after ,  [whitespace/comma] [3]
> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:553:  Missing space after ,  [whitespace/comma] [3]

Oops, fixed.

> Total errors found: 2 in 14 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I think this patch will break all of 32 bit.  Working on a fix.

[Filip Pizlo]

Created attachment 214125 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 214125 [details]
the patch

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

r=me

> Source/JavaScriptCore/jit/JIT.h:229
>              : m_type(PutById)
>              , bytecodeIndex(bytecodeIndex)
>              , hotPathBegin(hotPathBegin)
> +            , structureToCompare(structureToCompare)
>              , propertyStorageLoad(propertyStorageLoad)
> -            , putStructureToCompare(structureToCompare)
>  #if USE(JSVALUE64)
>              , putDisplacementLabel(displacementLabel)

You said you wanted to remove this constructor.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/157411

[Ryosuke Niwa]

This may have improved Dromaeo/jslib-event-jquery's score by ~28%:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%2C%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%5D&days=52&zoom=%5B1381585431357.5227%2C1381827392596%5D

[Ryosuke Niwa]

Created attachment 214291 [details]
PerformanceTestsResults for Dromaeo/jslib-event-jquery

(In reply to comment #13)
> This may have improved Dromaeo/jslib-event-jquery's score by ~28%:
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%2C%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%5D&days=52&zoom=%5B1381585431357.5227%2C1381827392596%5D

Confirmed.

[Gavin Barraclough]

nice!