| Summary: | Change JSC debug hooks to pass a CallFrame* instead of a DebuggerCallFrame. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> |
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | fpizlo, ggaren, joepeck, mhahnenberg, msaboff, oliver, timothy |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | |||
| Bug Blocks: | 121796 | ||
| Attachments: | |||
Created attachment 212500 [details]
the patch.
Comment on attachment 212500 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212500&action=review Looks good to me. Some nits and I'd prefer a JSC reviewer for the JSC parts. > Source/JavaScriptCore/debugger/Debugger.h:54 > + virtual void exception(CallFrame*, JSValue exceptionValue, bool hasHandler) = 0; > + virtual void atStatement(CallFrame*) = 0; > + virtual void callEvent(CallFrame*) = 0; > + virtual void returnEvent(CallFrame*) = 0; > + > + virtual void willExecuteProgram(CallFrame*) = 0; > + virtual void didExecuteProgram(CallFrame*) = 0; > + virtual void didReachBreakpoint(CallFrame*) = 0; Nit: Given the recent pointer -> reference effort that has been going on. Can these still take a reference since we know they will never be null? I have no strong preference, but that seems stricter. > Source/JavaScriptCore/debugger/DebuggerCallFrame.h:59 > + unsigned line() const > + { > + unsigned line, column; > + computeLineAndColumn(line, column); > + return line; > + } > + unsigned column() const > + { > + unsigned line, column; > + computeLineAndColumn(line, column); > + return column; > + } This seems like a recipe for duplicated work. (foo(frame->line(), frame->column() computes twice). Is this going away? Or maybe it is very efficient? How about just computing them once lazily and storing the values in member variables? > Source/WebCore/bindings/js/ScriptDebugServer.h:151 > + virtual void callEvent(JSC::CallFrame*); > + virtual void atStatement(JSC::CallFrame*); > + virtual void returnEvent(JSC::CallFrame*); > + virtual void exception(JSC::CallFrame*, JSC::JSValue exceptionValue, bool hasHandler); > + virtual void willExecuteProgram(JSC::CallFrame*); > + virtual void didExecuteProgram(JSC::CallFrame*); > + virtual void didReachBreakpoint(JSC::CallFrame*); Nit: Add OVERRIDE to all virtual methods. > Source/WebCore/bindings/js/WorkerScriptDebugServer.h:62 > virtual bool isContentScript(JSC::ExecState*) { return false; } > > - virtual void willExecuteProgram(const JSC::DebuggerCallFrame&); > + virtual void willExecuteProgram(JSC::CallFrame*); Nit: Add OVERRIDE to the virtual methods and probably FINAL to the class. > Source/WebKit/mac/WebView/WebScriptDebugger.h:65 > + virtual void callEvent(JSC::CallFrame*) { } > + virtual void atStatement(JSC::CallFrame*) { } > + virtual void returnEvent(JSC::CallFrame*) { } > + virtual void exception(JSC::CallFrame*, JSC::JSValue exceptionValue, bool hasHandler); > + virtual void willExecuteProgram(JSC::CallFrame*) { } > + virtual void didExecuteProgram(JSC::CallFrame*) { } > + virtual void didReachBreakpoint(JSC::CallFrame*) { } Nit: Add OVERRIDE / FINAL. FINAL on the class seems appropriate since these virtual methods are all private. Comment on attachment 212500 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212500&action=review >> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:59 >> + } > > This seems like a recipe for duplicated work. (foo(frame->line(), frame->column() computes twice). Is this going away? Or maybe it is very efficient? > > How about just computing them once lazily and storing the values in member variables? I agree. Even better would be to compute these values eagerly, so we don't have to bother with a state variable telling us where we've computed them yet. The client debugger will always want to know which line and column it's on, so there's no savings to being lazy. Mark, was this change motivated by some kind of performance measurement? Comment on attachment 212500 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212500&action=review > Source/WebCore/bindings/js/JavaScriptCallFrame.h:89 > - JavaScriptCallFrame(const JSC::DebuggerCallFrame&, PassRefPtr<JavaScriptCallFrame> caller); > + JavaScriptCallFrame(JSC::CallFrame*, PassRefPtr<JavaScriptCallFrame> caller); DebuggerCallFrame is an essential abstraction barrier. Please don't remove it! (In reply to comment #3) > (From update of attachment 212500 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212500&action=review > > >> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:59 > >> + } > > > > This seems like a recipe for duplicated work. (foo(frame->line(), frame->column() computes twice). Is this going away? Or maybe it is very efficient? > > > > How about just computing them once lazily and storing the values in member variables? > > I agree. Even better would be to compute these values eagerly, so we don't have to bother with a state variable telling us where we've computed them yet. The client debugger will always want to know which line and column it's on, so there's no savings to being lazy. > > Mark, was this change motivated by some kind of performance measurement? No, not for performance reasons. This was just so that we get the line and column info from the CallFrame as the single source of where we're executing instead of having to pass them via the debug hooks. For now, I didn't bother optimizing it. But sure, I can compute them eagerly. Created attachment 212519 [details]
JavaScriptCallFrame is now constructed with DebuggerCallFrame as an arg again. Added all the suggested FINAL and OVERRIDE clauses. Also cached line and column in DebuggerCallFrame.
However, I did not change the JSC debug hooks to pass a CallFrame& instead of the CallFrame*. This is in keeping with the convention of how we pass CallFrames in JSC. Eventually, these debug hook code should move back into JSC::Debugger, and no one outside of JSC will see them.
Comment on attachment 212519 [details]
JavaScriptCallFrame is now constructed with DebuggerCallFrame as an arg again. Added all the suggested FINAL and OVERRIDE clauses. Also cached line and column in DebuggerCallFrame.
r=me
Thanks for all the feedback and review. Landed in r156374: <http://trac.webkit.org/r156374>. |
And with this change, we should also: 1. Removed the need for passing the line and column info to the debug hook callbacks. We now get the line and column info from the CallFrame. 2. Simplify BytecodeGenerator::emitDebugHook() to only take 1 line number argument. The caller can determine whether to pass in the first or last line number of the block of source code as appropriate. Note: we still need pass in line and column info to emitDebugHook() because it uses this info to emit expression info which is used by the CallFrame to determine the line and column info for its "pc". 3. Pass the exceptionValue explicitly to the exception() debug hook callback. It should be embedded in the CallFrame / DebuggerCallFrame. 4. Change the op_debug opcode size to 2 (from 5) since we've remove 3 arg values. Update LLINT and JIT code to handle this.