Bug 122024

Summary: Compress DFG stack layout
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, buildbot, commit-queue, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, oliver, rakuco, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 122025, 122047, 122065, 122140, 122178, 122248    
Bug Blocks: 113621    
Attachments:
Description Flags
it begins
none
more!
none
it's coming along
none
more
none
even more
none
it runs things?
none
it runs more things
none
it only fails 41 tests!
none
it fails fewer tests?
none
dfg 64-bit works
none
almost done with FTL
none
victory!
none
the patch
oliver: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
getting close
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
patch for landing none

Description Filip Pizlo 2013-09-27 11:15:16 PDT
The DFG's usage of the stack is getting out of hand.  It will make it difficult to play nice with the FTL.  It's also probably already a performance bottleneck.

We should compress the stack layout such that:

- All stack slots that may be accessed out-of-line, like captured variables, arguments, scope/callee, etc., should be packed as close to the frame pointer as possible.

- All other stack slots should still be packed, but ought to be packed at higher frame offsets.
Comment 1 Filip Pizlo 2013-09-27 21:52:05 PDT
Created attachment 212876 [details]
it begins
Comment 2 Filip Pizlo 2013-09-30 19:42:17 PDT
Created attachment 213048 [details]
more!
Comment 3 Filip Pizlo 2013-09-30 22:26:20 PDT
Created attachment 213055 [details]
it's coming along
Comment 4 Filip Pizlo 2013-10-01 20:21:07 PDT
Created attachment 213144 [details]
more
Comment 5 Filip Pizlo 2013-10-01 22:10:11 PDT
Created attachment 213151 [details]
even more
Comment 6 Filip Pizlo 2013-10-02 13:40:28 PDT
Created attachment 213192 [details]
it runs things?

It's hugely broken but it's sort of getting there.
Comment 7 Filip Pizlo 2013-10-02 15:40:06 PDT
Created attachment 213208 [details]
it runs more things

It's still totally wrong but I can now run bigger programs than before.
Comment 8 Filip Pizlo 2013-10-02 15:54:42 PDT
Created attachment 213210 [details]
it only fails 41 tests!
Comment 9 Filip Pizlo 2013-10-02 21:25:37 PDT
Created attachment 213226 [details]
it fails fewer tests?
Comment 10 Filip Pizlo 2013-10-04 10:23:36 PDT
Created attachment 213371 [details]
dfg 64-bit works
Comment 11 Filip Pizlo 2013-10-04 23:06:06 PDT
Created attachment 213444 [details]
almost done with FTL
Comment 12 Filip Pizlo 2013-10-05 09:27:43 PDT
Created attachment 213454 [details]
victory!
Comment 13 WebKit Commit Bot 2013-10-05 09:29:36 PDT
Attachment 213454 [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.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/CodeOrigin.h', u'Source/JavaScriptCore/bytecode/Operands.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.cpp', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGCommonData.h', u'Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.cpp', u'Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.h', u'Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.cpp', u'Source/JavaScriptCore/dfg/DFGOSREntry.cpp', u'Source/JavaScriptCore/dfg/DFGOSREntry.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGScoreBoard.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/DFGStackLayoutPhase.cpp', u'Source/JavaScriptCore/dfg/DFGStackLayoutPhase.h', u'Source/JavaScriptCore/dfg/DFGValidate.cpp', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/StackVisitor.h', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/runtime/Arguments.h', u'Source/JavaScriptCore/runtime/JSActivation.h', u'Source/JavaScriptCore/runtime/JSFunction.cpp']" exit_code: 1
Source/JavaScriptCore/bytecode/CodeOrigin.h:101:  Please declare integral type bitfields with either signed or unsigned.  [runtime/bitfields] [5]
Source/JavaScriptCore/runtime/JSActivation.h:44:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Filip Pizlo 2013-10-05 09:35:14 PDT
Created attachment 213455 [details]
the patch

Passes all of the JSC tests.  Still going to do some performance tests and run full layout tests.
Comment 15 WebKit Commit Bot 2013-10-05 09:36:23 PDT
Attachment 213455 [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.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/CodeOrigin.h', u'Source/JavaScriptCore/bytecode/Operands.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.cpp', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGCommonData.h', u'Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.cpp', u'Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.h', u'Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.cpp', u'Source/JavaScriptCore/dfg/DFGOSREntry.cpp', u'Source/JavaScriptCore/dfg/DFGOSREntry.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGScoreBoard.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/DFGStackLayoutPhase.cpp', u'Source/JavaScriptCore/dfg/DFGStackLayoutPhase.h', u'Source/JavaScriptCore/dfg/DFGValidate.cpp', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/StackVisitor.h', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/runtime/Arguments.h', u'Source/JavaScriptCore/runtime/JSActivation.h', u'Source/JavaScriptCore/runtime/JSFunction.cpp']" exit_code: 1
Source/JavaScriptCore/bytecode/CodeOrigin.h:101:  Please declare integral type bitfields with either signed or unsigned.  [runtime/bitfields] [5]
Source/JavaScriptCore/runtime/JSActivation.h:44:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Oliver Hunt 2013-10-05 09:40:54 PDT
Comment on attachment 213455 [details]
the patch

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

>> Source/JavaScriptCore/bytecode/CodeOrigin.h:101
>> +    int stackOffset : 30;
> 
> Please declare integral type bitfields with either signed or unsigned.  [runtime/bitfields] [5]

bring back the signed - this rule is to deal with various compiler issues we've run into in the past (msvc mostly)
Comment 17 Filip Pizlo 2013-10-05 09:50:43 PDT
(In reply to comment #16)
> (From update of attachment 213455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213455&action=review
> 
> >> Source/JavaScriptCore/bytecode/CodeOrigin.h:101
> >> +    int stackOffset : 30;
> > 
> > Please declare integral type bitfields with either signed or unsigned.  [runtime/bitfields] [5]
> 
> bring back the signed - this rule is to deal with various compiler issues we've run into in the past (msvc mostly)

Ooops - yeah, will revert that.

Thanks for the review!
Comment 18 Build Bot 2013-10-05 10:17:51 PDT
Comment on attachment 213455 [details]
the patch

Attachment 213455 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3311222
Comment 19 Build Bot 2013-10-05 10:39:47 PDT
Comment on attachment 213455 [details]
the patch

Attachment 213455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3318226

New failing tests:
js/dom/dfg-arguments-alias-activation.html
inspector/console/console-format.html
inspector/console/command-line-api-inspect.html
inspector/console/command-line-api.html
inspector/console/console-native-function-to-string.html
Comment 20 Build Bot 2013-10-05 10:39:50 PDT
Created attachment 213459 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 21 Build Bot 2013-10-05 11:37:43 PDT
Comment on attachment 213455 [details]
the patch

Attachment 213455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3333044

New failing tests:
js/dom/dfg-arguments-alias-activation.html
inspector/console/console-format.html
inspector/console/command-line-api-inspect.html
inspector/console/command-line-api.html
inspector/console/console-native-function-to-string.html
Comment 22 Build Bot 2013-10-05 11:37:46 PDT
Created attachment 213464 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 23 Build Bot 2013-10-05 11:45:49 PDT
Comment on attachment 213455 [details]
the patch

Attachment 213455 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3323043

New failing tests:
js/dom/dfg-arguments-alias-activation.html
inspector/console/console-format.html
inspector/console/command-line-api-inspect.html
inspector/console/command-line-api.html
inspector/console/console-native-function-to-string.html
Comment 24 Build Bot 2013-10-05 11:45:52 PDT
Created attachment 213465 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 25 Filip Pizlo 2013-10-05 18:02:23 PDT
(In reply to comment #23)
> (From update of attachment 213455 [details])
> Attachment 213455 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/3323043
> 
> New failing tests:
> js/dom/dfg-arguments-alias-activation.html
> inspector/console/console-format.html
> inspector/console/command-line-api-inspect.html
> inspector/console/command-line-api.html
> inspector/console/console-native-function-to-string.html

These are legit and I'm working on fixes.
Comment 26 Filip Pizlo 2013-10-05 18:48:08 PDT
Created attachment 213479 [details]
getting close
Comment 27 WebKit Commit Bot 2013-10-05 18:50:19 PDT
Attachment 213479 [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.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/CodeOrigin.h', u'Source/JavaScriptCore/bytecode/Operands.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.cpp', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGCommonData.h', u'Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.cpp', u'Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.h', u'Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.cpp', u'Source/JavaScriptCore/dfg/DFGOSREntry.cpp', u'Source/JavaScriptCore/dfg/DFGOSREntry.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGScoreBoard.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/DFGStackLayoutPhase.cpp', u'Source/JavaScriptCore/dfg/DFGStackLayoutPhase.h', u'Source/JavaScriptCore/dfg/DFGValidate.cpp', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/StackVisitor.h', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/Arguments.h', u'Source/JavaScriptCore/runtime/JSActivation.h', u'Source/JavaScriptCore/runtime/JSFunction.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/JSActivation.h:44:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/CodeBlock.h:353:  The parameter name "inlineCallFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Build Bot 2013-10-05 19:28:01 PDT
Comment on attachment 213479 [details]
getting close

Attachment 213479 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3474004
Comment 29 Build Bot 2013-10-05 19:50:40 PDT
Comment on attachment 213479 [details]
getting close

Attachment 213479 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3472010

New failing tests:
js/dom/dfg-arguments-alias-activation.html
inspector/console/console-format.html
inspector/console/command-line-api-inspect.html
inspector/console/command-line-api.html
inspector/console/console-native-function-to-string.html
Comment 30 Build Bot 2013-10-05 19:50:43 PDT
Created attachment 213483 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 31 Filip Pizlo 2013-10-05 20:37:18 PDT
Created attachment 213486 [details]
patch for landing

Seems like all the bugs are fixed - the previous test crashes and failures were because StackLayoutPhase was using local indices as virtual registers for creating the machine slow arguments.  Yuck.
Comment 32 Filip Pizlo 2013-10-05 21:22:01 PDT
Landed in http://trac.webkit.org/changeset/156984