Bug 122024 – Compress DFG stack layout

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 212876 [details]
it begins

[Filip Pizlo]

Created attachment 213048 [details]
more!

[Filip Pizlo]

Created attachment 213055 [details]
it's coming along

[Filip Pizlo]

Created attachment 213144 [details]
more

[Filip Pizlo]

Created attachment 213151 [details]
even more

[Filip Pizlo]

Created attachment 213192 [details]
it runs things?

It's hugely broken but it's sort of getting there.

[Filip Pizlo]

Created attachment 213208 [details]
it runs more things

It's still totally wrong but I can now run bigger programs than before.

[Filip Pizlo]

Created attachment 213210 [details]
it only fails 41 tests!

[Filip Pizlo]

Created attachment 213226 [details]
it fails fewer tests?

[Filip Pizlo]

Created attachment 213371 [details]
dfg 64-bit works

[Filip Pizlo]

Created attachment 213444 [details]
almost done with FTL

[Filip Pizlo]

Created attachment 213454 [details]
victory!

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 213455 [details]
the patch

Passes all of the JSC tests.  Still going to do some performance tests and run full layout tests.

[WebKit Commit Bot]

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.

[Oliver Hunt]

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)

[Filip Pizlo]

(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!

[Build Bot]

Comment on attachment 213455 [details]
the patch

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 213479 [details]
getting close

[WebKit Commit Bot]

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.

[Build Bot]

Comment on attachment 213479 [details]
getting close

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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