Bug 121749

Summary:

Move DFG inline caching logic into jit/

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, commit-queue, ggaren, gyuyoung.kim, jonlee, mark.lam, mhahnenberg, msaboff, oliver, rakuco, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

121641, 122102

Attachments:

Description	Flags
the patch	sam: review+
the patch	none
revised patch: rebased to latest revision, and attempted fixes for Windows build.	none
patch 4: more Windows build fixes.	none
patch 5: one more Windows build fix.	ggaren: review+, ggaren: commit-queue-
patch 6: svn up'ed to see if that fixes the merge issue on the EWS bots	none
patch 7: one more after rollout of r156474 which broke the 32-bt build.	none
patch 8: one more time since the EWS bots seems to be working again now.	none

Description Filip Pizlo 2013-09-21 12:14:24 PDT

Patch forthcoming.

Comment 1 Filip Pizlo 2013-09-21 13:16:06 PDT

Created attachment 212281 [details]
the patch

Comment 2 WebKit Commit Bot 2013-09-21 13:17:24 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/CallLinkInfo.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/StructureStubInfo.h', u'Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.h', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGRegisterSet.h', u'Source/JavaScriptCore/dfg/DFGRepatch.cpp', u'Source/JavaScriptCore/dfg/DFGRepatch.h', u'Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGThunks.cpp', u'Source/JavaScriptCore/dfg/DFGThunks.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/RegisterSet.h', u'Source/JavaScriptCore/jit/Repatch.cpp', u'Source/JavaScriptCore/jit/Repatch.h', u'Source/JavaScriptCore/jit/ScratchRegisterAllocator.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.h']" exit_code: 1
Source/JavaScriptCore/jit/Repatch.cpp:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:561:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:722:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:759:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:796:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:880:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:117:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:130:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:145:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:156:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:177:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:188:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/Repatch.h:26:  #ifndef header guard has wrong style, please use: Repatch_h  [build/header_guard] [5]
Total errors found: 17 in 33 files


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

Comment 3 Sam Weinig 2013-09-21 13:42:02 PDT

Comment on attachment 212281 [details]
the patch

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

> Source/JavaScriptCore/jit/JITCall.cpp:135
>  void JIT::compileCallEval(Instruction* instruction)
>  {
> +    
>      JITStubCall stubCall(this, cti_op_call_eval); // Initializes ScopeChain; ReturnPC; CodeBlock.

You didn't mean to add this line.

> Source/JavaScriptCore/jit/Repatch.h:27
> +#define DFGRepatch_h

You should fix the header guard.

Comment 4 Sam Weinig 2013-09-21 13:42:13 PDT

rs=me.

Comment 5 Filip Pizlo 2013-09-21 13:45:57 PDT

(In reply to comment #3)
> (From update of attachment 212281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212281&action=review
> 
> > Source/JavaScriptCore/jit/JITCall.cpp:135
> >  void JIT::compileCallEval(Instruction* instruction)
> >  {
> > +    
> >      JITStubCall stubCall(this, cti_op_call_eval); // Initializes ScopeChain; ReturnPC; CodeBlock.
> 
> You didn't mean to add this line.

Word.

> 
> > Source/JavaScriptCore/jit/Repatch.h:27
> > +#define DFGRepatch_h
> 
> You should fix the header guard.

Yeah.

Comment 6 Filip Pizlo 2013-09-21 16:10:31 PDT

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

Comment 7 Jon Lee 2013-09-21 16:54:33 PDT

This broke the Windows port:
http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/52505

Comment 8 Filip Pizlo 2013-09-21 17:05:27 PDT

(In reply to comment #7)
> This broke the Windows port:
> http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/52505

Fixing...

Comment 9 Filip Pizlo 2013-09-21 17:19:53 PDT

(In reply to comment #8)
> (In reply to comment #7)
> > This broke the Windows port:
> > http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/52505
> 
> Fixing...

Actually, I will revert.  There's no way to fix Windows.

Comment 10 Filip Pizlo 2013-09-21 17:57:28 PDT

Created attachment 212289 [details]
the patch

Comment 11 Filip Pizlo 2013-09-21 17:57:50 PDT

...

Comment 12 WebKit Commit Bot 2013-09-21 18:00:35 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/CallLinkInfo.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/StructureStubInfo.h', u'Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.h', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGRegisterSet.h', u'Source/JavaScriptCore/dfg/DFGRepatch.cpp', u'Source/JavaScriptCore/dfg/DFGRepatch.h', u'Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGThunks.cpp', u'Source/JavaScriptCore/dfg/DFGThunks.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.h', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/RegisterSet.h', u'Source/JavaScriptCore/jit/Repatch.cpp', u'Source/JavaScriptCore/jit/Repatch.h', u'Source/JavaScriptCore/jit/ScratchRegisterAllocator.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.h']" exit_code: 1
Source/JavaScriptCore/jit/Repatch.cpp:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:561:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:722:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:759:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:796:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:880:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:124:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:137:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:152:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:163:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:184:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:195:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 34 files


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

Comment 13 Mark Lam 2013-09-25 20:16:17 PDT

Created attachment 212656 [details]
revised patch: rebased to latest revision, and attempted fixes for Windows build. 

Uploading this patch so that we can try this on a Windows build.

Comment 14 Mark Lam 2013-09-25 20:22:35 PDT

(In reply to comment #13)
> Created an attachment (id=212656) [details]
> revised patch: rebased to latest revision, and attempted fixes for Windows build. 
> 
> Uploading this patch so that we can try this on a Windows build.

Patch 3 is still failing to build on Windows.  Investigating.

Comment 15 Mark Lam 2013-09-25 21:21:41 PDT

Created attachment 212660 [details]
patch 4: more Windows build fixes.

Comment 16 Mark Lam 2013-09-25 22:20:21 PDT

Created attachment 212665 [details]
patch 5: one more Windows build fix.

Comment 17 Mark Lam 2013-09-25 22:40:48 PDT

Comment on attachment 212665 [details]
patch 5: one more Windows build fix.

Patch 5 builds jsc properly although I encountered other build issues in WebCore that are unrelated to this patch.  That said, I think this patch should be reviewed before I land it.  The changes that I made in addition to the original patch from http://trac.webkit.org/changeset/156235 are:

1. Rebase up to the latest svn revision.
2. Added FUNCTION_WRAPPER_WITH_RETURN_ADDRESS macros for COMPILER(MSVC) && CPU(X86).
3. Moved implementations of getHostCallReturnValue and getHostCallReturnValueWithExecState to JITOperations.cpp from DFGOperations.cpp.  These pieces of code needs to be built even if the DFG is not enabled for build.  Hence, they don't belong in DFGOperations.cpp.
4. I added an implementation of getHostCallReturnValue for COMPILER(MSVC) && CPU(X86).

Comment 18 Geoffrey Garen 2013-09-26 08:20:47 PDT

Comment on attachment 212665 [details]
patch 5: one more Windows build fix.

r=me, assuming it builds and passes tests.

Can you do a pass with the EWS bot before landing? It looks like this version of the patch didn't apply cleanly on the EWS bot.

Comment 19 Mark Lam 2013-09-26 11:04:44 PDT

Created attachment 212726 [details]
patch 6: svn up'ed to see if that fixes the merge issue on the EWS bots

Comment 20 Mark Lam 2013-09-26 11:35:46 PDT

Created attachment 212729 [details]
patch 7: one more after rollout of r156474 which broke the 32-bt build.

Comment 21 Mark Lam 2013-09-26 12:13:27 PDT

Created attachment 212735 [details]
patch 8: one more time since the EWS bots seems to be working again now.

Comment 22 Mark Lam 2013-09-26 12:40:51 PDT

It looks like the EWS patching system is unable to deal with the file deletions and additions that this patch has.  I've already tries many times, and it doesn't seem to make any difference no matter how many times I svn up to the latest.

The patch builds and runs jsc.exe properly on Windows.  I was not able to run testapi.exe as it crashes due to some pre-existing bug.

I'm not able to build all of WebKit because of a pre-existing build failure in WebCore/platform/graphics/cg/GraphicsContextCG.cpp even without this patch.

I'll proceed with landing this patch and deal with any fallout if needed.

Comment 23 Geoffrey Garen 2013-09-26 13:15:27 PDT

> I was not able to run testapi.exe as it crashes due to some pre-existing bug.

Do we have a bug filed for this?

Comment 24 Mark Lam 2013-09-26 13:20:21 PDT

(In reply to comment #23)
> > I was not able to run testapi.exe as it crashes due to some pre-existing bug.
> 
> Do we have a bug filed for this?

I will file a bug.

Just ran run-javascriptcore-tests on Windows with testapi omitted, and it passed.  However, it only ran the Mozilla suite.  It didn't run the LayoutTests and stress test suites as it would have done on Mac.  This is without me making any changes to omit those other tests.

Comment 25 Mark Lam 2013-09-26 13:38:40 PDT

Thanks for the review.  Landed in r156490: <http://trac.webkit.org/r156490>.

FYI, bug for the testapi crash: https://bugs.webkit.org/show_bug.cgi?id=121972.
Bug for the WebCore build errors: https://bugs.webkit.org/show_bug.cgi?id=121974.