Bug 122836

Summary: Poor man's fast breakpoints for a 2.3x debugger speedup
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: burg, fpizlo, ggaren, joepeck, mhahnenberg, msaboff, oliver, timothy, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 121796, 124245, 127127, 127230, 127269, 127272, 127321    
Bug Blocks: 122847    
Attachments:
Description Flags
the patch.
none
patch 2: added a comment.
none
patch 3: revised with more efficient globalObject check in ApplyBreakpointFunctor and ClearAllBreakpointsFunctor.
none
patch 4: fix one more inefficient globalObject check in an assertion in Debugger::hasBreakpoint().
ggaren: review+
updated patch for landing. none

Mark Lam
Reported 2013-10-15 09:21:02 PDT
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.
Attachments
the patch. (31.97 KB, patch)
2014-01-21 23:31 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 2: added a comment. (32.11 KB, patch)
2014-01-21 23:38 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 3: revised with more efficient globalObject check in ApplyBreakpointFunctor and ClearAllBreakpointsFunctor. (32.09 KB, patch)
2014-01-22 03:03 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 4: fix one more inefficient globalObject check in an assertion in Debugger::hasBreakpoint(). (32.08 KB, patch)
2014-01-22 03:12 PST, Mark Lam 		ggaren: review+
DetailsFormatted DiffDiff
updated patch for landing. (32.08 KB, patch)
2014-01-22 23:33 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-10-15 09:21:36 PDT
<rdar://problem/15231325>
Mark Lam
Comment 2 2013-10-15 10:34:43 PDT
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
Comment 3 2013-11-06 16:17:44 PST
Removing 122838 as a dependency since it was a pre-existing issue that is independent of the debugger.
Mark Lam
Comment 4 2013-11-06 16:21:29 PST
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
Comment 5 2014-01-21 20:30:06 PST
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
Comment 6 2014-01-21 22:09:00 PST
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
Comment 7 2014-01-21 23:31:38 PST
Created attachment 221831 [details] the patch.
Mark Lam
Comment 8 2014-01-21 23:38:26 PST
Created attachment 221832 [details] patch 2: added a comment.
Mark Lam
Comment 9 2014-01-22 03:03:24 PST
Created attachment 221850 [details] patch 3: revised with more efficient globalObject check in ApplyBreakpointFunctor and ClearAllBreakpointsFunctor.
Mark Lam
Comment 10 2014-01-22 03:12:49 PST
Created attachment 221851 [details] patch 4: fix one more inefficient globalObject check in an assertion in Debugger::hasBreakpoint().
Timothy Hatcher
Comment 11 2014-01-22 09:57:03 PST
(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
Comment 12 2014-01-22 10:10:34 PST
*** Bug 122844 has been marked as a duplicate of this bug. ***
Geoffrey Garen
Comment 13 2014-01-22 19:22:43 PST
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
Comment 14 2014-01-22 23:31:26 PST
(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
Comment 15 2014-01-22 23:33:55 PST
Created attachment 221950 [details] updated patch for landing.
Mark Lam
Comment 16 2014-01-22 23:36:13 PST
Landed in r162598: <http://trac.webkit.org/r162598>.
Note You need to log in before you can comment on or make changes to this bug.