Summary: | Avoid JSC debugger overhead unless needed | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, joepeck, mhahnenberg, msaboff, oliver, sebastian, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mark Lam
2013-10-20 08:16:13 PDT
Created attachment 214702 [details]
the patch.
This patch builds and works for the intended purpose. However, I still need to run regression tests on it.
Comment on attachment 214702 [details]
the patch.
One more change needed. Revised patch coming soon.
Comment on attachment 214702 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=214702&action=review > Source/JavaScriptCore/debugger/Debugger.cpp:95 > + : m_isStepping(false) Let's call this m_needsOpDebugCallbacks. > Source/JavaScriptCore/debugger/Debugger.cpp:96 > + , m_needStepping(false) Let's call this m_shouldPause. > Source/JavaScriptCore/debugger/Debugger.cpp:98 > + , m_needExceptionEvents(false) Let's call this m_needsExceptionCallbacks. > Source/JavaScriptCore/debugger/Debugger.h:82 > + friend class JIT; > + friend class LLIntOffsetsExtractor; You shouldn't need to make JIT a friend. Instead, you should provide a function named "needsOpDebugCallbacksOffset()", which returns the offset of that data member. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1057 > + loadPtr(&debugger->m_isStepping, regT0); Can we just use a bool with branchTest8? > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:803 > + loadp JSGlobalObject::m_debugger[t0], t0 > + btiz t0, .opDebugDone You shouldn't need to null-check the debugger. The JIT doesn't do so, so doing so here will only mask bugs. (In reply to comment #4) > > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:803 > > + loadp JSGlobalObject::m_debugger[t0], t0 > > + btiz t0, .opDebugDone > > You shouldn't need to null-check the debugger. The JIT doesn't do so, so doing so here will only mask bugs. Some layout tests are showing that I do need to null-check the debugger. This is needed when the debugger has been detached but the VM hasn't recompiled all the functions yet. In my testing I see the following order of functions called: 1. WebCore::ScriptDebugServer::recompileAllJSFunctionsSoon(this=0x0000000108131380) + 19 at ScriptDebugServer.cpp:628 2. JSC::Debugger::detach(this=0x0000000108131380, globalObject=0x000000010e31f970) + 24 at Debugger.cpp:118 But before we get to JSC::Debugger::recompileAllJSFunctions(JSC::VM*) + 23 at Debugger.cpp:138, I see: 3. * thread #1: tid = 0x17e96f, 0x00000001005a9a6d JavaScriptCore`llint_op_debug + 15, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0x30) frame #0: 0x00000001005a9a6d JavaScriptCore`llint_op_debug + 15 JavaScriptCore`llint_op_debug + 15: -> 0x1005a9a6d: movzbl 48(%rax), %eax 0x1005a9a71: testb %al, %al 0x1005a9a73: je 0x1005a9a98 ; llint_op_debug + 58 0x1005a9a79: leaq (%r10,%rsi,8), %rsi where %rax is 0 but is supposed to contain the Debugger*. So, instead of removing this check from the LLINT, I'll add it to the JIT generated code. Created attachment 214753 [details]
patch 2: addressed Geoff's feedback, enable exception callbacks always for the Mac WebScriptDebugger, and fixed a bunch of layout test failures.
This patch runs the layout tests with no known regressions.
Attachment 214753 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/debugger/Debugger.cpp', u'Source/JavaScriptCore/debugger/Debugger.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebScriptDebugger.mm']" exit_code: 1
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1057: Declaration has space between type name and * in char *debuggerAddress [whitespace/declaration] [3]
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1060: Declaration has space between type name and * in char *flagAddress [whitespace/declaration] [3]
Source/JavaScriptCore/jit/JITOpcodes.cpp:733: Declaration has space between type name and * in char *debuggerAddress [whitespace/declaration] [3]
Source/JavaScriptCore/jit/JITOpcodes.cpp:735: Declaration has space between type name and * in char *flagAddress [whitespace/declaration] [3]
Total errors found: 4 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 214756 [details]
patch 3: fixed style issues.
Comment on attachment 214756 [details] patch 3: fixed style issues. View in context: https://bugs.webkit.org/attachment.cgi?id=214756&action=review r=me > Source/JavaScriptCore/ChangeLog:10 > + - Fixed a bug with ScriptDebugServer::m_currentCallFrame getting stale. Your ChangeLog needs to explain what this bug was, how you fixed it, and what it's tested by. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:261 > + setNeedsExceptionCallbacks(pause != DontPauseOnExceptions); The default Web Inspector condition is to require callbacks for uncaught exceptions, but not for caught exceptions. It's not very interesting to have a debugger feature for when no exception callbacks are required at all, since that condition is false by default. It would be more interesting to have a debugger feature for when only uncaught exception callbacks are required. Thanks for the review. The ChangeLog comment has been updated to describe why m_currentCallFrame can get stale and how it is corrected. Landed in r157746: <http://trac.webkit.org/r157746>. Does this handle the case where a debugger statement is hit but there are no breakpoints attached (and it's not set to pause on the next statement)? I don't see a case for that. I'm also having trouble with the llint version of this check. It skips the debug hook when it shouldn't. Not sure why. (In reply to comment #11) > Does this handle the case where a debugger statement is hit but there are no breakpoints attached (and it's not set to pause on the next statement)? I don't see a case for that. > > I'm also having trouble with the llint version of this check. It skips the debug hook when it shouldn't. Not sure why. Hmmm, I had forgotten about the debugger statement. I’ll look into it. > Does this handle the case where a debugger statement is hit but there are no breakpoints attached (and it's not set to pause on the next statement)?
I think the behavior we want here is to treat the debugger statement as if it were a breakpoint.
|