Bug 147374 – Implement try/catch in the DFG.

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

[Saam Barati]

This should be a big win in terms of removing barriers to tier up.

More of my implementation plan to follow as I familiarize myself
with the code.

[Saam Barati]

Created attachment 258380 [details]
Super rough WIP

Super rough and crazy, but it's a start.
This program works:

function assert(b) {
    if (!b)
        throw new Error("Bad value");
}
noInline(assert);

function bar(a, b, c) {
    try {
        assert(a + b === c);
    } catch(e) {
        print("caught exception in bar " + e);
    }
}

function foo(a, b, c) {
    try {
        bar(a, b, c);
        assert(a + b === c);
    } catch(e) {
        print("caught exception in foo " + e);
    }
}

var start = preciseTime();
noInline(foo);
for (var i = 0; i < 100000; i++) {
    foo(10, 20, 30);
}
foo(10, 20, 40);


var end = preciseTime();
print(end - start);

[Saam Barati]

Created attachment 258729 [details]
WIP

still rough but more programs are working. About ~50 test failures currently, but we need more test coverage, which I'm adding as I go.
This patch still doesn't include renaming/refactoring of things.

[Saam Barati]

Created attachment 259110 [details]
WIP

still rough. The patch for bug #147988 is subsumed into this patch until #147988 lands.

[Saam Barati]

Created attachment 261041 [details]
WIP

getting much closer to being done.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.h:266:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.h:277:  The parameter name "linkBuffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:135:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:264:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp:67:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp:113:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp:138:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp:153:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:74:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:78:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:927:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1912:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1913:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1914:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1915:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:273:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:278:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3555:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:599:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 19 in 41 files


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

[Build Bot]

Comment on attachment 261041 [details]
WIP

Attachment 261041 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/163316

New failing tests:
fast/workers/worker-close.html

[Build Bot]

Created attachment 261052 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 261141 [details]
patch

I still want to write a few more tests but I think it's ready for comments/suggestions.

[Filip Pizlo]

Comment on attachment 261141 [details]
patch

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

> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:422
> +    <ClInclude Include="..\dfg\DFGFlushLiveCatchVariablesInsertionPhase.cpp" />

better: "LiveCatchVariableFlushInsertionPhase"

"performLiveCatchVariableFlushInsertion" reads better than "performFlushLiveCatchVariablesInsertion"

[Filip Pizlo]

Comment on attachment 261141 [details]
patch

As part of this patch, I'd like to see one of raytrace, richards, deltablue, or earley-boyer converted to have try{}catch{} everyhwere.  I think that this is important to prove the speed-up and also to introduce test coverage.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.h:282:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:132:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:264:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:187:  Alphabetical sorting problem. "dfg/DFGFlushLiveCatchVariablesInsertionPhase.cpp" should be before "dfg/DFGFlushedAt.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 7 in 45 files


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

[Michael Saboff]

Comment on attachment 261141 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:94
> +            //restoreCalleeSavesFromVMCalleeSavesBuffer();

I think you need to uncomment this line.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:104
> +

Delete this line.

> Source/JavaScriptCore/dfg/DFGPlan.cpp:264
> +    //dfg.dump();

Debug code?

[Saam Barati]

Created attachment 261236 [details]
patch

[Saam Barati]

Comment on attachment 261236 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:157
> +            for (InlineCallFrame* inlineCallFrame : seenInlineCallFrames)

This seems a little hacky to me. What do you guys think?

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.h:282:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:132:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:70:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:141:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 58 files


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

[Filip Pizlo]

Comment on attachment 261236 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:157
>> +            for (InlineCallFrame* inlineCallFrame : seenInlineCallFrames)
> 
> This seems a little hacky to me. What do you guys think?

No, this seems fine!

> Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.h:35
> +// FIXME: comment here.

:-)  You should add a comment here!

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/189938

[Ryosuke Niwa]

Looks like this patch broke LLint Cloop build:
https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=50

[Ryosuke Niwa]

Fixed the build in http://trac.webkit.org/changeset/189952.  Please verify the correctness.

[Alexey Proskuryakov]

This broke workers:

https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20(Tests)/r189954%20(7554)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149329

[Alexey Proskuryakov]

There may have been memory corruption on non-worker tests too (but you can't now with memory corruption): https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r189957%20(15679)/js/regress/richards-try-catch-crash-log.txt

[Saam Barati]

Created attachment 261531 [details]
patch for landing

I want to let EWS take a look.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:132:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:137:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 62 files


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

[Saam Barati]

landed take 2 fix in:
http://trac.webkit.org/changeset/189995