Bug 122836 – Poor man's fast breakpoints for a 2.3x debugger speedup

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

[Mark Lam]

A quick experiment to comment out the debug hook callbacks (to the ScriptDebugServer in Interpreter::debug()) reveals a 2.3x improvement in the Octane benchmark score.

To harness this 2.3x speedup quickly, we can employ the following strategy:

1. Add a "needBreaks" operand to the op_debug opcode.  needBreaks is 0 by default.  The debugger will set it to a non-zero value if a breakpoint is needed at that op_debug opcode. 

2. Implement a Debugger::setBreakpoint() that will set the "needBreaks" operand of the appropriate op_debug instruction.
    2.1 Add a look up function to the ClassBlock to find the bytecodeOffset of the appropriate op_debug opcode for a given line and column position. 
    2.2 Add a Debugger::m_sourceIdToExecutable map that allows the debugger to find the Executable and ClassBlock to set a breakpoint in.

3. Modify sourceParsed() notifications / infrastructure to do the following:
    3.2 Register Executables in the Debugger::m_sourceIdToExecutable map.
    3.1 set any relevant unset breakpoints as needed (Note: breakpoints may be requested before the Executable / ClassBlock is created).

4. Modify the ScriptExecutable's destructor to unregister itself with the relevant debugger if needed.

5. Modify the Debugger to track 2 mode of execution:
    5.1 Running: only breaks (and calls the debug hook callbacks) when it sees an op_debug with a needBreaks flag set.  Otherwise, it does not call the debug hook callbacks.
    5.2 Stepping: will call every debug hook callback.

    Stepping mode is entered as soon as Running mode breaks.  Running mode resumes when the Debugger::continueExecution() is called.

This effort aims to get some significant speedup (2.3x) with minimal effort but lays the ground work for the Debugger:
1. to identify breakpoints by bytecode offset.
2. to distinguish between Running and Stepping modes.
3. to track all the Source / Executable / ClassBlock it is debugging.

Work is in progress.

[Radar WebKit Bug Importer]

<rdar://problem/15231325>

[Mark Lam]

FYI, this implementation is meant to only work with the LLINT and the baseline JIT.  The DFG is not yet enabled (i.e. no change from the current debugger implementation).

[Mark Lam]

Removing 122838 as a dependency since it was a pre-existing issue that is independent of the debugger.

[Mark Lam]

This implementation needs a Breakpoint tracking mechanism to record the FunctionExecutables and bytecodeOffsets where breakpoints are set.  Rather than introducing a separate breakpoint tracking mechanism, we should wrap up https://bugs.webkit.org/show_bug.cgi?id=121796 which moves the breakpoint tracking mechanism into the JSC::Debugger.  Adding 121796 as a blocking dependency.

[Mark Lam]

Instead of bytecode level breakpoints, we're changing the plan to using a m_numBreakpoints count in CodeBlocks as the indicator that op_debug callbacks are needed.  This limits the perf penalty of op_debug callbacks to only CodeBlocks that have breakpoints set in them.  As a result, we can stick with the existing breakpoint setting and checking mechanisms for now.

[Mark Lam]

Results so far running Octane 2.0 at http://octane-benchmark.googlecode.com/svn/latest/index.html with the WebInspector opened (higher score is better):

r162390 baseline w/ no breakpoints: 1949
r162390 baseline w/ breakpoints:      401
r162390 patched w/ no breakpoints: 2024
r162390 patched w/ breakpoints:      2034

The set breakpoint is set in a function that is not relevant to the benchmark run.  The same breakpoint location on both the baseline and patched runs.
 
The ~100 point difference between the patched run and the baseline runs is due to noise, not because of any perf progression.

[Mark Lam]

Created attachment 221831 [details]
the patch.

[Mark Lam]

Created attachment 221832 [details]
patch 2: added a comment.

[Mark Lam]

Created attachment 221850 [details]
patch 3: revised with more efficient globalObject check in ApplyBreakpointFunctor and ClearAllBreakpointsFunctor.

[Mark Lam]

Created attachment 221851 [details]
patch 4: fix one more inefficient globalObject check in an assertion in Debugger::hasBreakpoint().

[Timothy Hatcher]

(In reply to comment #6)
> Results so far running Octane 2.0 at http://octane-benchmark.googlecode.com/svn/latest/index.html with the WebInspector opened (higher score is better):
> 
> r162390 baseline w/ no breakpoints: 1949
> r162390 baseline w/ breakpoints:      401
> r162390 patched w/ no breakpoints: 2024
> r162390 patched w/ breakpoints:      2034
> 
> The set breakpoint is set in a function that is not relevant to the benchmark run.  The same breakpoint location on both the baseline and patched runs.
> 
> The ~100 point difference between the patched run and the baseline runs is due to noise, not because of any perf progression.

Impressive! Testing how it performs with an auto-continue breakpoint that gets hit would also be interesting.

[Mark Lam]

*** Bug 122844 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

Comment on attachment 221851 [details]
patch 4: fix one more inefficient globalObject check in an assertion in Debugger::hasBreakpoint().

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

r=me, with some fixups below.

> Source/JavaScriptCore/ChangeLog:63
> +          This is a N * log(N) algorithm, but it is assumed that in a nominal
> +          system, we won't be calling CodeBlock::hasOpDebugForLineAndColumn() to
> +          impact interactive debugger performance. If this should prove to be
> +          untrue, there are several optimization options that we can entertain
> +          in the future to address this.

Let's not assume. Instead, load a website, set a breakpoint, and test whether doing so takes a noticeable amount of time -- either by eye, or by printf(). Then, put your findings in this ChangeLog.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1359
>          case op_debug: {
>              int debugHookID = (++it)->u.operand;
> +            int hasBreakpointFlag = (++it)->u.operand;
>              printLocationAndOp(out, exec, location, it, "debug");
> -            out.printf("%s", debugHookName(debugHookID));
> +            out.printf("%s %d", debugHookName(debugHookID), hasBreakpointFlag);
>              break;
>          }

Is this field used? I don't think so. Perhaps we should remove it in a follow-up patch -- unless you plan to start using it soon.

> Source/JavaScriptCore/debugger/Debugger.cpp:212
> +bool Debugger::applyBreakpointInCodeBlockIfNeeded(CodeBlock* codeBlock, Breakpoint& breakpoint, bool enableBreakpoint)

This should return void.

Let's call this "toggleBreakpoint". Let's call the argument "enabledOrNot".

> Source/JavaScriptCore/debugger/Debugger.cpp:254
> +void Debugger::iterateBreakpointsAndApplyToCodeBlock(CodeBlock* codeBlock)

Let's call this "applyBreakpoints".

> Source/JavaScriptCore/debugger/Debugger.cpp:259
> +        applyBreakpointInCodeBlockIfNeeded(codeBlock, breakpoint, true);

WebKit style says you should declare "true" on a separate line with the name "enabledOrNot". The raw "true" is unreadable without more context.

> Source/JavaScriptCore/debugger/Debugger.cpp:263
> +class ApplyBreakpointFunctor {

Let's call this "ToggleBreakpointFunctor".

You should put this functor class in an anonymous namespace to avoid name conflicts with classes declared in other files. Alternatively, you can define it in the Debugger class.

> Source/JavaScriptCore/debugger/Debugger.cpp:285
> +void Debugger::iterateCodeBlocksAndApplyBreakpoint(Breakpoint& breakpoint, bool enableOrNot)

Let's call this "toggleBreakpoint".

> Source/JavaScriptCore/debugger/Debugger.cpp:294
> +class ClearAllBreakpointsFunctor {

Anonymous namespace or class scope, please.

> Source/JavaScriptCore/debugger/Debugger.cpp:312
> +void Debugger::iterateCodeBlocksAndClearAllBreakpoints()

Let's call this "clearBreakpoints".

> Source/JavaScriptCore/debugger/Debugger.cpp:370
> +    iterateCodeBlocksAndApplyBreakpoint(breakpoint, true);

Declare "true" in named variable, please.

> Source/JavaScriptCore/debugger/Debugger.cpp:390
> +    iterateCodeBlocksAndApplyBreakpoint(breakpoint, false);

Declare "false" in named variable, please.

> Source/JavaScriptCore/heap/CodeBlockSet.h:81
> +        HashSet<CodeBlock*>::iterator iter = m_set.begin();
> +        HashSet<CodeBlock*>::iterator end = m_set.end();
> +        for (; iter != end; ++iter) {

Please use C++11 range syntax here.

> Source/JavaScriptCore/runtime/Executable.cpp:176
> +        if (debugger)
> +            debugger->unregisterCodeBlock(oldCodeBlock.get());

Let's not unregister.

If the old code never runs again, we wasted time by unregistering. If the old code does run again, we want it to be debuggable. In either case, there are no dangling pointers to worry about.

[Mark Lam]

(In reply to comment #13)
> (From update of attachment 221851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221851&action=review
> 
> r=me, with some fixups below.
> 
> > Source/JavaScriptCore/ChangeLog:63
> > +          This is a N * log(N) algorithm, but it is assumed that in a nominal
> > +          system, we won't be calling CodeBlock::hasOpDebugForLineAndColumn() to
> > +          impact interactive debugger performance. If this should prove to be
> > +          untrue, there are several optimization options that we can entertain
> > +          in the future to address this.
> 
> Let's not assume. Instead, load a website, set a breakpoint, and test whether doing so takes a noticeable amount of time -- either by eye, or by printf(). Then, put your findings in this ChangeLog.

The interactive performance appears to be on par with the baseline before this patch is applied.  I updated the ChangeLog to indicate this.

> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1359
> >          case op_debug: {
> >              int debugHookID = (++it)->u.operand;
> > +            int hasBreakpointFlag = (++it)->u.operand;
> >              printLocationAndOp(out, exec, location, it, "debug");
> > -            out.printf("%s", debugHookName(debugHookID));
> > +            out.printf("%s %d", debugHookName(debugHookID), hasBreakpointFlag);
> >              break;
> >          }
> 
> Is this field used? I don't think so. Perhaps we should remove it in a follow-up patch -- unless you plan to start using it soon.

Not currently used.  I want to fix it up here for correctness.  We can remove it in a subsequent patch if we don't decide to use it.

> > Source/JavaScriptCore/debugger/Debugger.cpp:212
> > +bool Debugger::applyBreakpointInCodeBlockIfNeeded(CodeBlock* codeBlock, Breakpoint& breakpoint, bool enableBreakpoint)
> 
> This should return void.

Fixed.

> Let's call this "toggleBreakpoint". Let's call the argument "enabledOrNot".

Fixed.

> > Source/JavaScriptCore/debugger/Debugger.cpp:254
> > +void Debugger::iterateBreakpointsAndApplyToCodeBlock(CodeBlock* codeBlock)
> 
> Let's call this "applyBreakpoints".

Fixed.

> > Source/JavaScriptCore/debugger/Debugger.cpp:259
> > +        applyBreakpointInCodeBlockIfNeeded(codeBlock, breakpoint, true);
> 
> WebKit style says you should declare "true" on a separate line with the name "enabledOrNot". The raw "true" is unreadable without more context.

Created a Debugger::BreakpointState enum to use as the argument instead.  BreakpointState will have 2 values: BreakpointDisabled and BreakpointEnabled.

> > Source/JavaScriptCore/debugger/Debugger.cpp:263
> > +class ApplyBreakpointFunctor {
> 
> Let's call this "ToggleBreakpointFunctor".

Fixed.
 
> You should put this functor class in an anonymous namespace to avoid name conflicts with classes declared in other files. Alternatively, you can define it in the Debugger class.

Fixed.  Made this a private class in Debugger.
 
> > Source/JavaScriptCore/debugger/Debugger.cpp:285
> > +void Debugger::iterateCodeBlocksAndApplyBreakpoint(Breakpoint& breakpoint, bool enableOrNot)
> 
> Let's call this "toggleBreakpoint".

Fixed.

> > Source/JavaScriptCore/debugger/Debugger.cpp:294
> > +class ClearAllBreakpointsFunctor {
> 
> Anonymous namespace or class scope, please.

Fixed.  Made this a private class in Debugger.

> > Source/JavaScriptCore/debugger/Debugger.cpp:312
> > +void Debugger::iterateCodeBlocksAndClearAllBreakpoints()
> 
> Let's call this "clearBreakpoints".

Fixed.  There's already a function called clearBreakpoints() which is the sole caller of this function.  I folded this function into the caller.  I also renamed ClearAllBreakpointsFunctor to ClearBreakpointsFunctor to match clearBreakpoints(). 
 
> > Source/JavaScriptCore/debugger/Debugger.cpp:370
> > +    iterateCodeBlocksAndApplyBreakpoint(breakpoint, true);
> 
> Declare "true" in named variable, please.

Fixed.  Using BreakpointEnabled.

> > Source/JavaScriptCore/debugger/Debugger.cpp:390
> > +    iterateCodeBlocksAndApplyBreakpoint(breakpoint, false);
> 
> Declare "false" in named variable, please.

Fixed.  Using BreakpointDisabled.

> > Source/JavaScriptCore/heap/CodeBlockSet.h:81
> > +        HashSet<CodeBlock*>::iterator iter = m_set.begin();
> > +        HashSet<CodeBlock*>::iterator end = m_set.end();
> > +        for (; iter != end; ++iter) {
> 
> Please use C++11 range syntax here.

Fixed.

> > Source/JavaScriptCore/runtime/Executable.cpp:176
> > +        if (debugger)
> > +            debugger->unregisterCodeBlock(oldCodeBlock.get());
> 
> Let's not unregister.
> 
> If the old code never runs again, we wasted time by unregistering. If the old code does run again, we want it to be debuggable. In either case, there are no dangling pointers to worry about.

Fixed.

[Mark Lam]

Created attachment 221950 [details]
updated patch for landing.

[Mark Lam]

Landed in r162598: <http://trac.webkit.org/r162598>.