Bug 158498

Summary: Simplify Interpreter::StackFrame.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, fpizlo, ggaren, keith_miller, msaboff, rniwa, saam
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
mark.lam: review-, buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite
none
new and improved.
saam: review+
Patch for landing. none

Mark Lam
Reported 2016-06-07 15:25:37 PDT
Previously, Interpreter::StackFrame (which is used to capture info for Error.stack) eagerly extracts info out of CodeBlock and duplicate the work that CodeBlock does to compute line and column numbers (amongst other things). This patch does away with the eager extraction and only stashes the CodeBlock pointer in the Interpreter::StackFrame. Instead, Interpreter::StackFrame will provide methods for computing the desired values on request later.
Attachments
proposed patch. (18.23 KB, patch)
2016-06-07 15:29 PDT, Mark Lam
mark.lam: review-
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite (854.96 KB, application/zip)
2016-06-07 16:27 PDT, Build Bot
no flags
new and improved. (18.20 KB, patch)
2016-06-08 11:42 PDT, Mark Lam
saam: review+
Patch for landing. (18.65 KB, patch)
2016-06-08 13:00 PDT, Mark Lam
no flags
Mark Lam
Comment 1 2016-06-07 15:29:03 PDT
Created attachment 280741 [details] proposed patch.
Build Bot
Comment 2 2016-06-07 16:26:57 PDT
Comment on attachment 280741 [details] proposed patch. Attachment 280741 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1462327 New failing tests: fast/events/attribute-listener-deletion-crash.html html5lib/generated/run-tests7-write.html html5lib/generated/run-tests1-write.html html5lib/generated/run-tests18-write.html fast/dom/attribute-event-listener-errors.html fast/events/window-onerror-syntax-error-in-attr.html fast/text/text-combine-crash.html
Build Bot
Comment 3 2016-06-07 16:27:02 PDT
Created attachment 280744 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Mark Lam
Comment 4 2016-06-07 16:29:38 PDT
Comment on attachment 280741 [details] proposed patch. Taking out of review while I look at the failures. Might just need to rebase some tests.
Mark Lam
Comment 5 2016-06-08 11:42:58 PDT
Created attachment 280819 [details] new and improved.
Saam Barati
Comment 6 2016-06-08 12:42:22 PDT
Comment on attachment 280819 [details] new and improved. View in context: https://bugs.webkit.org/attachment.cgi?id=280819&action=review > Source/JavaScriptCore/ChangeLog:9 > + Error.stack) eagerly extracts info out of CodeBlock and duplicate the work that s/duplicate/duplicates > Source/JavaScriptCore/interpreter/Interpreter.cpp:120 > + } Style: I would add a ``` default: ASSERT_NOT_REACHED() ``` case > Source/JavaScriptCore/interpreter/Interpreter.cpp:524 > + StackFrame s = { > + Strong<JSObject>(vm, visitor->callee()), > + Strong<CodeBlock>(), > + 0 > + }; Style: I would say unsigned bytecodeOffset = 0; and then pass that into the constructor. (Or as I said before, make this an Optional<unsigned> > Source/JavaScriptCore/runtime/Error.cpp:151 > + StackFrame* stackFrame; I would call this firstNonNativeFrame. > Source/JavaScriptCore/runtime/Error.cpp:154 > for (unsigned i = 0 ; i < stackTrace.size(); ++i) { > stackFrame = &stackTrace.at(i); > if (stackFrame->bytecodeOffset) As we discussed in person, it's worth changing this to !stackFrame->isNative() Using bytecodeOffset == 0 is just weird and confusing because the bytecodeOffset is allowed to be zero. If anything, we can make it an Optional<unsigned>. > Source/JavaScriptCore/runtime/ErrorInstance.cpp:147 > + bool hasTrace = addErrorInfoAndGetBytecodeOffset(exec, vm, this, useCurrentFrame, callFrame, hasSourceAppender() ? &bytecodeOffset : nullptr); nice change, the previous code was quite an anti-pattern.
Mark Lam
Comment 7 2016-06-08 12:56:58 PDT
Comment on attachment 280819 [details] new and improved. View in context: https://bugs.webkit.org/attachment.cgi?id=280819&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + Error.stack) eagerly extracts info out of CodeBlock and duplicate the work that > > s/duplicate/duplicates Fixed. > Source/JavaScriptCore/ChangeLog:31 > + In the old code, the Error object's addErrorInfoAndGetBytecodeOffset() and the > + inspector's createScriptCallStackFromException() would check if sourceURL is > + empty. If so, they will use this as an indicator to use alternate source info > + in the Error object e.g. url and line numbers from the parser that produced a > + SyntaxError. > + > + In the new implementation, StackFrame only has a sourceURL() function that > + behaves like the old friendlySourceURL(). The client code which were relying > + on sourceURL being empty, will now explicitly check if the StackFrame is for > + native code using a new isNative() query in addition to the sourceURL being > + empty. This achieve functional parity with the old behavior. I indented these 2 paragraphs and added some text to the previous paragraph to make it clearer that these are details elaborating on that previous paragraph. >> Source/JavaScriptCore/interpreter/Interpreter.cpp:120 >> + } > > Style: I would add a > ``` > default: > ASSERT_NOT_REACHED() > ``` > case Applied. >> Source/JavaScriptCore/interpreter/Interpreter.cpp:524 >> + }; > > Style: I would say > unsigned bytecodeOffset = 0; > and then pass that into the constructor. > (Or as I said before, make this an Optional<unsigned> I added a "// unused value because codeBlock is null." comment next to the 0. >> Source/JavaScriptCore/runtime/Error.cpp:151 >> + StackFrame* stackFrame; > > I would call this firstNonNativeFrame. Applied. >> Source/JavaScriptCore/runtime/Error.cpp:154 >> if (stackFrame->bytecodeOffset) > > As we discussed in person, it's worth changing this to !stackFrame->isNative() > Using bytecodeOffset == 0 is just weird and confusing because the bytecodeOffset is allowed to be zero. > If anything, we can make it an Optional<unsigned>. I changed this to: if (!firstNonNativeFrame->isNative()) > Source/JavaScriptCore/runtime/Error.cpp:173 > + if (!stackFrame->isNative() && !frameSourceURL.isEmpty()) Now that we realize that the above loop is looking for the first non-native frame, then we know that this "!stackFrame->isNative()" is superflous. I removed it.
Mark Lam
Comment 8 2016-06-08 13:00:42 PDT
Created attachment 280826 [details] Patch for landing.
Mark Lam
Comment 9 2016-06-08 14:04:30 PDT
Thanks for the review. Landed in r201830: <http://trac.webkit.org/r201830>.
Note You need to log in before you can comment on or make changes to this bug.