Bug 116889

Summary: fourthTier: The baseline jit and LLint should use common slow paths
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 116888    
Attachments:
Description Flags
Work in progress
none
Patch
none
Updated patch addressing reviewer comments
none
Patch with redundant hit stubs removed
fpizlo: review+
Patch for landing with minor merge up changes. none

Description Michael Saboff 2013-05-28 14:38:31 PDT
To reduce the number of trampoline methods and generally clean up the code in preparation for utilizing the C stack, we should reduce the number of slow paths.  This change is to eliminate nearly identical slow paths.
Comment 1 Michael Saboff 2013-05-28 14:55:17 PDT
Created attachment 203095 [details]
Work in progress

This patch compiles and runs correctly on X86 and X86_64.  Working through ARMv7 issues.
Comment 2 Michael Saboff 2013-05-28 16:41:41 PDT
Created attachment 203106 [details]
Patch

Builds and passes JSC tests on X86_64 and X86.  Speculative fixes for other platforms.
Comment 3 Mark Lam 2013-05-28 17:32:52 PDT
Comment on attachment 203106 [details]
Patch

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

Do you have benchmark numbers to confirm that this does not impact perf?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:716
> +//    ASSERT(callFrame == vm->topCallFrame || callFrame == callFrame->lexicalGlobalObject()->globalExec() || callFrame == callFrame->dynamicGlobalObject()->globalExec());

Why comment out this assert?

> Source/JavaScriptCore/jit/JITExceptions.h:43
> +    // 

Why the blank comment?
Comment 4 Michael Saboff 2013-05-29 16:44:06 PDT
Created attachment 203289 [details]
Updated patch addressing reviewer comments

I had commented out the ASSERT during development.  Fixed the reason that it was failing on 32_64 builds by adding in the storePtr(callFrameRegister, &m_vm->topCallFrame); back in at the end of privateCompileCTINativeCall().

Also removed the bogus empty comment.
Comment 5 Filip Pizlo 2013-06-01 10:14:25 PDT
Comment on attachment 203289 [details]
Updated patch addressing reviewer comments

Shouldn't there be a bunch of stuff removed from JITStubs.cpp?
Comment 6 Michael Saboff 2013-06-03 11:56:30 PDT
Created attachment 203617 [details]
Patch with redundant hit stubs removed

(In reply to comment #5)
> (From update of attachment 203289 [details])
> Shouldn't there be a bunch of stuff removed from JITStubs.cpp?

Removed.
Comment 7 Michael Saboff 2013-06-07 16:45:30 PDT
Created attachment 204074 [details]
Patch for landing with minor merge up changes.
Comment 8 Michael Saboff 2013-06-07 16:46:37 PDT
Committed r151342: <http://trac.webkit.org/changeset/151342>
Comment 9 Filip Pizlo 2013-06-07 17:00:26 PDT
I'm pretty sure this broke Kraken in debug build.
Comment 10 Filip Pizlo 2013-06-07 19:12:35 PDT
Rolled out in http://trac.webkit.org/changeset/151345
Comment 11 Michael Saboff 2013-06-07 21:12:48 PDT
(In reply to comment #9)
> I'm pretty sure this broke Kraken in debug build.

Looks like there were a couple of emitSlow_op_*() functions where I didn't load the return register (rax on X86_64) with the result of the operation after the call.  I fixed the ones I found and kraken works.  I'll spend a little more time testing before relanding.
Comment 12 Geoffrey Garen 2013-06-08 09:53:44 PDT
This is blocking the patch I'm working on, since I built on top of JITSlowPathCall :(.
Comment 13 Geoffrey Garen 2013-06-09 22:17:37 PDT
I took your diagnosis and fixed and re-landed this patch in <http://trac.webkit.org/changeset/151362>.
Comment 14 Geoffrey Garen 2013-06-10 13:35:23 PDT
Darn, looks like this is still crashing at r151362:

run-webkit-tests --debug --verbose fast/workers/stress-js-execution.html fast/workers/termination-early.html fast/workers/stress-js-execution.html fast/workers/termination-early.html

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef

Thread 14 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore      	0x000000010954c9d0 JSC::JSActivation::tearOff(JSC::VM&) + 96 (JSActivation.h:154)
1   com.apple.JavaScriptCore      	0x0000000109547466 JSC::Interpreter::unwindCallFrame(JSC::ExecState*&, JSC::JSValue, unsigned int&, JSC::CodeBlock*&) + 550 (Interpreter.cpp:499)
2   com.apple.JavaScriptCore      	0x0000000109548841 JSC::Interpreter::throwException(JSC::ExecState*&, JSC::JSValue&, unsigned int) + 1137 (Interpreter.cpp:779)
3   com.apple.JavaScriptCore      	0x000000010956be06 JSC::genericThrow(JSC::VM*, JSC::ExecState*, JSC::JSValue, unsigned int) + 166 (JITExceptions.cpp:56)
4   com.apple.JavaScriptCore      	0x000000010956bf6c JSC::jitThrowNew(JSC::VM*, JSC::ExecState*, JSC::JSValue) + 60 (JITExceptions.cpp:80)
5   com.apple.JavaScriptCore      	0x000000010958f37c cti_vm_throw_slowpath + 60 (JITStubs.cpp:2230)
6   com.apple.JavaScriptCore      	0x0000000109584db9 ctiVMThrowTrampolineSlowpath + 8
Comment 15 Geoffrey Garen 2013-06-10 14:15:59 PDT
Rolled out in r151401.
Comment 16 Michael Saboff 2013-06-12 11:30:23 PDT
Fixed test failure and rolled back in with change set <https://trac.webkit.org/changeset/151504>

Fixed involved NOT processing exceptions through the llint path when we call a common slow path from the JIT.