Bug 147374 - Implement try/catch in the DFG.
Summary: Implement try/catch in the DFG.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on: 147666 147988 149228 149329
Blocks:
  Show dependency treegraph
 
Reported: 2015-07-28 14:15 PDT by Saam Barati
Modified: 2015-09-18 16:38 PDT (History)
12 users (show)

See Also:


Attachments
Super rough WIP (21.91 KB, patch)
2015-08-06 11:29 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (24.33 KB, patch)
2015-08-11 11:35 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (141.71 KB, patch)
2015-08-15 17:34 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (56.65 KB, patch)
2015-09-11 19:40 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (614.19 KB, application/zip)
2015-09-12 02:28 PDT, Build Bot
no flags Details
patch (59.95 KB, patch)
2015-09-14 15:27 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (167.33 KB, patch)
2015-09-15 14:18 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing (192.35 KB, patch)
2015-09-18 15:47 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-07-28 14:15:26 PDT
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.
Comment 1 Saam Barati 2015-08-06 11:29:29 PDT
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);
Comment 2 Saam Barati 2015-08-11 11:35:16 PDT
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.
Comment 3 Saam Barati 2015-08-15 17:34:58 PDT
Created attachment 259110 [details]
WIP

still rough. The patch for bug #147988 is subsumed into this patch until #147988 lands.
Comment 4 Saam Barati 2015-09-11 19:40:13 PDT
Created attachment 261041 [details]
WIP

getting much closer to being done.
Comment 5 WebKit Commit Bot 2015-09-12 00:50:45 PDT
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.
Comment 6 Build Bot 2015-09-12 02:28:32 PDT
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
Comment 7 Build Bot 2015-09-12 02:28:36 PDT
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
Comment 8 Saam Barati 2015-09-14 15:27:39 PDT
Created attachment 261141 [details]
patch

I still want to write a few more tests but I think it's ready for comments/suggestions.
Comment 9 Filip Pizlo 2015-09-14 15:29:45 PDT
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"
Comment 10 Filip Pizlo 2015-09-14 15:31:28 PDT
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.
Comment 11 WebKit Commit Bot 2015-09-14 15:32:22 PDT
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.
Comment 12 Michael Saboff 2015-09-14 16:04:30 PDT
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?
Comment 13 Saam Barati 2015-09-15 14:18:09 PDT
Created attachment 261236 [details]
patch
Comment 14 Saam Barati 2015-09-15 14:18:51 PDT
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?
Comment 15 WebKit Commit Bot 2015-09-15 14:20:08 PDT
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.
Comment 16 Filip Pizlo 2015-09-15 14:23:30 PDT
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!
Comment 17 Saam Barati 2015-09-17 15:03:51 PDT
landed in:
http://trac.webkit.org/changeset/189938
Comment 18 Ryosuke Niwa 2015-09-17 17:45:56 PDT
Looks like this patch broke LLint Cloop build:
https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=50
Comment 19 Ryosuke Niwa 2015-09-17 17:56:23 PDT
Fixed the build in http://trac.webkit.org/changeset/189952.  Please verify the correctness.
Comment 21 WebKit Commit Bot 2015-09-17 19:52:32 PDT
Re-opened since this is blocked by bug 149329
Comment 22 Alexey Proskuryakov 2015-09-17 20:32:56 PDT
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
Comment 23 Saam Barati 2015-09-18 15:47:22 PDT
Created attachment 261531 [details]
patch for landing

I want to let EWS take a look.
Comment 24 WebKit Commit Bot 2015-09-18 15:50:20 PDT
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.
Comment 25 Saam Barati 2015-09-18 16:38:30 PDT
landed take 2 fix in:
http://trac.webkit.org/changeset/189995