Bug 127532 – Removing the need for Debugger* and m_shouldPause op_debug check

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

[Mark Lam]

Remove the speculation checks for debugger and "shouldPause", and change the debugger to add a breakpoint to every code block when it starts stepping, and remove them all when it stops stepping.

[Mark Lam]

Created attachment 222082 [details]
work in progress

The plan is to replace the m_shouldPause by marking every relevant  CodeBlock to indicate that op_debug callbacks are needed.  However, the Octane perf results with this change so far shows a significant regression:

baseline w/ WebInspector + breakpoint:   2600
patched w/ WebInspector + breakpoint:   1540

I need to look thru this patch again for potential bugs, and also do some profiling to see why it is regressing the Octane score.  Uploading this patch to archive the work so far.

[Mark Lam]

Comment on attachment 222082 [details]
work in progress

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

> Source/JavaScriptCore/bytecode/CodeBlock.h:1031
> +            unsigned m_steppingMode : 1;

Found 1 mistake.  I forgot to initialize m_steppingMode in the constructor.  Once that was fixed, the Octane score came up to 2840.  That's a progression.  I'll see if there are any other bugs before I redo the benchmarks and get the final comparison.

[Mark Lam]

Created attachment 222084 [details]
patch 2: fixed initialization bug.

This patch fixes the initialization of CodeBlock::m_steppingMode.  The Octane scores are now:

baseline w/ WebInspector + breakpoint:   2640
patched w/ WebInspector + breakpoint:    2830    1.07x progression

This patched has passed the layout tests.

[Geoffrey Garen]

Comment on attachment 222084 [details]
patch 2: fixed initialization bug.

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        with a procedure to toffle a SteppingMode flag on all CodeBlocks under

"toggle"

> Source/JavaScriptCore/bytecode/CodeBlock.h:1032
> +            signed m_numBreakpoints : 31;

Why is this signed?

> Source/JavaScriptCore/debugger/Debugger.cpp:193
> +    clearDebuggerRequestsInGlobal(globalObject);

No need for "InGlobal" in the name. The global object parameter is a part of the function signature.

> Source/JavaScriptCore/debugger/Debugger.cpp:223
> +void Debugger::toggleSteppingMode(SteppingMode mode)

This should be "setSteppingMode". Same for other uses of "toggle" in this patch.

[Geoffrey Garen]

Comment on attachment 222084 [details]
patch 2: fixed initialization bug.

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

> Source/JavaScriptCore/debugger/Debugger.cpp:236
>  void Debugger::registerCodeBlock(CodeBlock* codeBlock)

Don't you need to update this function to apply stepping mode to a newly-created CodeBlock?

What happens when I step into a function that has never been compiled before? When it's compiled, how does its CodeBlock get the right value for m_steppingMode?

> Source/JavaScriptCore/debugger/Debugger.cpp:503
> +class Debugger::ClearCodeBlockDebuggerRequestsInGlobalFunctor {

This can just be "ClearDebuggerRequestsFunctor".

[Mark Lam]

Thanks for the review.

(In reply to comment #4)
> > Source/JavaScriptCore/ChangeLog:9
> > +        with a procedure to toffle a SteppingMode flag on all CodeBlocks under
> 
> "toggle"

Will fix.
 
> > Source/JavaScriptCore/bytecode/CodeBlock.h:1032
> > +            signed m_numBreakpoints : 31;
> 
> Why is this signed?

Because the count should never get that big anyway, and we allow for negatives so that we can assert that we never try to “remove” more breakpoints than we “add”.  On second thought, never mind.  I’ll assert that the count is not 0 before I “remove” a breakpoint.  I’ll change this to unsigned.
 
> > Source/JavaScriptCore/debugger/Debugger.cpp:193
> > +    clearDebuggerRequestsInGlobal(globalObject);
> 
> No need for "InGlobal" in the name. The global object parameter is a part of the function signature.

Will fix.


> > Source/JavaScriptCore/debugger/Debugger.cpp:223
> > +void Debugger::toggleSteppingMode(SteppingMode mode)
> 
> This should be "setSteppingMode". Same for other uses of "toggle" in this patch.

Will fix.


(In reply to comment #5)
> (From update of attachment 222084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222084&action=review
> 
> > Source/JavaScriptCore/debugger/Debugger.cpp:236
> >  void Debugger::registerCodeBlock(CodeBlock* codeBlock)
> 
> Don't you need to update this function to apply stepping mode to a newly-created CodeBlock?
> 
> What happens when I step into a function that has never been compiled before? When it's compiled, how does its CodeBlock get the right value for m_steppingMode?

Will fix.

> > Source/JavaScriptCore/debugger/Debugger.cpp:503
> > +class Debugger::ClearCodeBlockDebuggerRequestsInGlobalFunctor {
> 
> This can just be "ClearDebuggerRequestsFunctor".

Will fix.

[Mark Lam]

Created attachment 222118 [details]
patch for landing: all fixes have been applied.

[Geoffrey Garen]

Comment on attachment 222118 [details]
patch for landing: all fixes have been applied.

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

r=me

> Source/JavaScriptCore/debugger/Debugger.cpp:245
>  void Debugger::unregisterCodeBlock(CodeBlock* codeBlock)
>  {
> -    codeBlock->clearAllBreakpoints();
> +    codeBlock->clearDebuggerRequests();

I think in a previous patch review I suggested removing this function and its callers altogether.

[Mark Lam]

(In reply to comment #8)
> > Source/JavaScriptCore/debugger/Debugger.cpp:245
> >  void Debugger::unregisterCodeBlock(CodeBlock* codeBlock)
> >  {
> > -    codeBlock->clearAllBreakpoints();
> > +    codeBlock->clearDebuggerRequests();
> 
> I think in a previous patch review I suggested removing this function and its callers altogether.

I removed the callers, but had left this function in.   Since there are no callers, I’ll go ahead and remove it as well before landing the current patch.

[Mark Lam]

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