[webkit-reviews] review granted: [Bug 122836] Poor man's fast breakpoints for a 2.3x debugger speedup : [Attachment 221851] patch 4: fix one more inefficient globalObject check in an assertion in Debugger::hasBreakpoint().

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 122836: Poor man's fast breakpoints for a 2.3x debugger speedup
https://bugs.webkit.org/show_bug.cgi?id=122836

Attachment 221851: patch 4: fix one more inefficient globalObject check in an
assertion in Debugger::hasBreakpoint().
https://bugs.webkit.org/attachment.cgi?id=221851&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.