Bug 123084 – Avoid JSC debugger overhead unless needed

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

[Mark Lam]

As a first step towards reclaiming baseline JIT level performance, we can disable debug hook callbacks if there are no breakpoints set.  If there are any breakpoints set, then performance will be where it was.  If all breakpoints are not set, then performance will improve by about 3x over shipping Safari.  Removing previously set breakpoints will also allow the performance to improve again.

In addition, we can also avoid exception event callbacks if break on all and uncaught exceptions is not enabled.  This doesn't significantly help with performance because exceptions are rare.

Will also address problems with ScriptDebugeServer::m_currentCallFrame getting stale.  This is needed in order for the above to work correctly.

Patch coming soon.

[Radar WebKit Bug Importer]

<rdar://problem/15273499>

[Mark Lam]

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.

[Mark Lam]

Comment on attachment 214702 [details]
the patch.

One more change needed.  Revised patch coming soon.

[Geoffrey Garen]

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.

[Mark Lam]

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

[Mark Lam]

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.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 214756 [details]
patch 3: fixed style issues.

[Geoffrey Garen]

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.

[Mark Lam]

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

[Sebastian Markbåge]

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.

[Mark Lam]

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

[Geoffrey Garen]

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