* SUMMARY Inspector Hanging under tryParseDestructuringPatternExpression. * STEPS TO REPRODUCE 1. Inspect <http://bogojoker.com/shell/> 2. Show Timeline Tab 3. Reload => Timeline starts a bit, then hangs completely * NOTES - Sample shows stuck under tryParseDestructuringPatternExpression: > 8033 void IPC::handleMessage<Messages::WebInspectorUI::SendMessageToFrontend, WebKit::WebInspectorUI, void (WebKit::WebInspectorUI::*)(WTF::String const&)>(IPC::MessageDecoder&, WebKit::WebInspectorUI*, void (WebKit::WebInspectorUI::*)(WTF::String const&)) (in WebKit) + 67 [0x10e37e1f7] HandleMessage.h:16 > 8033 WebKit::WebInspectorFrontendAPIDispatcher::dispatchMessageAsync(WTF::String const&) (in WebKit) + 72 [0x10e3761c0] WebInspectorFrontendAPIDispatcher.cpp:76 > 8033 WebKit::WebInspectorFrontendAPIDispatcher::evaluateExpressionOnLoad(WTF::String const&) (in WebKit) + 59 [0x10e375fd1] WebInspectorFrontendAPIDispatcher.cpp:83 > 8033 WebCore::ScriptController::executeScript(WTF::String const&, bool, WebCore::ExceptionDetails*) (in WebCore) + 343 [0x110a049a7] ScriptController.cpp:180 > 8033 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) (in WebCore) + 304 [0x110a02ca0] JSMainThreadExecState.h:62 > 8033 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) (in JavaScriptCore) + 469 [0x10f11ca75] Completion.cpp:104 > 8033 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) (in JavaScriptCore) + 10276 [0x10f441114] Interpreter.cpp:938 > 8033 JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*) (in JavaScriptCore) + 106 [0x10f32f47a] Executable.cpp:574 > 8033 JSC::JSGlobalObject::createProgramCodeBlock(JSC::ExecState*, JSC::ProgramExecutable*, JSC::JSObject**) (in JavaScriptCore) + 205 [0x10f4dafbd] JSGlobalObject.cpp:935 > 8033 JSC::CodeCache::getProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&) (in JavaScriptCore) + 82 [0x10f0f6a52] CodeCache.cpp:135 > 8033 JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::ThisTDZMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&, JSC::VariableEnvironment const*) (in JavaScriptCore) + 225 [0x10f0f71d1] CodeCache.cpp:104 > 8033 std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::parse<JSC::ProgramNode>(JSC::VM*, JSC::SourceCode const&, JSC::Identifier const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::SourceParseMode, JSC::ParserError&, JSC::JSTextPosition*, JSC::ConstructorKind, JSC::ThisTDZMode) (in JavaScriptCore) + 306 [0x10f0a7ae2] Parser.h:1428 > 8033 std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::ProgramNode>(JSC::ParserError&, JSC::Identifier const&, JSC::SourceParseMode) (in JavaScriptCore) + 96 [0x10f0a7d40] Parser.h:1342 > 8033 JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode) Parser.cpp:275 > 8033 JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode) Parser.cpp:377 > 8033 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatementListItem<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) Parser.cpp:499 > 8033 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) Parser.cpp:1532 > 8033 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionOrLabelStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2150 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2691 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3486 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128 > 8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2727 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::tryParseDestructuringPatternExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::AssignmentContext) Parser.cpp:771 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDestructuringPattern<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:913 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentElement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:793 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128 > 8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3255 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128 > 8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2727 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::tryParseDestructuringPatternExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::AssignmentContext) Parser.cpp:771 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDestructuringPattern<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:845 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentElement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:793 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128 > 8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128 > 8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948 > 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2727 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::tryParseDestructuringPatternExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::AssignmentContext) Parser.cpp:771 > 8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDestructuringPattern<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:913 > > ...
Created attachment 265629 [details] [TEST] Test Case Attached a test case. This test used to take 8ms to load on my machine, it now takes 13seconds. The test case just does: <script> Does.not.exist({...huge object literal...}) </script> So it is valid syntax, but immediate runtime error. Tons of time is spent parsing the literal.
We may want to roll out until the performance issue is addressed. I didn't see anything obviously wrong in the original change.
The cause is pretty straight forward, when parsing the destructuring pattern, there's a SavePoint for each property in the object literal, when parsed in an expression context, and this happens for every ObjectLiteral/ArrayLiteral parsed in an AssignmentExpression context. A fix that may work is re-organizing it to parse as an ObjectLiteral/ArrayLiteral first, and switch to destructuring mode only if followed by a "=".
Created attachment 265679 [details] Speed up AssignmentPattern parsing I haven't run the full test suite, this isn't ready yet, but it does produce a significant speedup for loading this page. Most real-world destructuring patterns are probably not that big, so while those would still be slow, they shouldn't be as noticeable. I'll add further fixes to the test suite as needed. PS, webkit-patch is failing to upload my patches today (https://gist.github.com/caitp/27af5c48cd1758c65784 for a log), so this is just the output from `git format-patch`, hope that's cool.
(In reply to comment #4) > Created attachment 265679 [details] > Speed up AssignmentPattern parsing > > I haven't run the full test suite, this isn't ready yet, but it does produce > a significant speedup for loading this page. Most real-world destructuring > patterns are probably not that big, so while those would still be slow, they > shouldn't be as noticeable. This seems like a good choice. You should add a similar test case to Joe's as a regression test. > PS, webkit-patch is failing to upload my patches today > (https://gist.github.com/caitp/27af5c48cd1758c65784 for a log), so this is > just the output from `git format-patch`, hope that's cool. We've been having server issues. As long as the patch applies, it doesn't matter how it's generated. Without webkit-patch, you will need to manually set flags for r?, cq?, and submitting it to EWS by clicking the button next to the attachment.
Created attachment 265692 [details] Patch
Comment on attachment 265692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265692&action=review > Source/JavaScriptCore/ChangeLog:3 > +Speed up Destructuring Assignment parsing, by only performing it if parsing Seems this ChangeLog formatting broke. Format like the other ChangeLogs below (bug title + reviewer at top, description beneath it, and indented). Also, can we get a test for this.
Comment on attachment 265692 [details] Patch Attachment 265692 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/442423 New failing tests: inspector/runtime/parse.html
Created attachment 265698 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 265699 [details] Patch Add a regression test and fix ChangeLog
These seem like legit minor issues with the patch caught by inspector/runtime/parse.html: > -PASS: Should be SyntaxErrorType Recoverable. > +FAIL: Should be SyntaxErrorType Recoverable. > Source: var x = {prop:123, > ^ > -PASS: Should be SyntaxErrorType Recoverable. > +FAIL: Should be SyntaxErrorType Recoverable. > Source: var x = [1, > ^ We expect the parser to be reporting that these are Recoverable syntax errors.
Comment on attachment 265699 [details] Patch Attachment 265699 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/442545 New failing tests: inspector/runtime/parse.html
Created attachment 265701 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 265703 [details] Patch Hack to fix the inspector test failure
Comment on attachment 265703 [details] Patch r=me
Comment on attachment 265703 [details] Patch Oops! Still failing parse.html.
Created attachment 265722 [details] Patch Really fix the failing inspector test
Created attachment 265765 [details] Proposal for further reducing re-parsing For consideration, this followup provides a way to reduce re-parsing even more
Comment on attachment 265722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265722&action=review > Source/JavaScriptCore/tests/stress/regress-151324.js:1698 > +})`, `new ReferenceError("Can't find variable: InspectorFrontendAPI")`); I think you need a newline at the end of this test to make diffing tools happy.
Comment on attachment 265765 [details] Proposal for further reducing re-parsing View in context: https://bugs.webkit.org/attachment.cgi?id=265765&action=review This seems like a more efficient approach. Can you merge it into your previous patch and rebase to trunk? > Source/JavaScriptCore/parser/Parser.cpp:2727 > + ExpressionErrorClassifier classification(this); Let's call this classifier to match its class name. > Source/JavaScriptCore/parser/Parser.h:740 > + enum ErrorClass { Let's call this ExpressionErrorClass. > Source/JavaScriptCore/parser/Parser.h:748 > + , m_outerClassifier(parser->m_errorClassifier) Let's call this m_previous. > Source/JavaScriptCore/parser/Parser.h:759 > + void classifyError(ErrorClass classification) Let's call this classifyExpressionError.
Comment on attachment 265722 [details] Patch Clearing this from request queue. Hey Caitlin, got time to follow up with Geoff's feedback?
This was landed in a separate bug (https://bugs.webkit.org/show_bug.cgi?id=151026) I don't have editbugs on webkit, but feel free to mark this one as resolved
(In reply to comment #22) > This was landed in a separate bug > (https://bugs.webkit.org/show_bug.cgi?id=151026) > > I don't have editbugs on webkit, but feel free to mark this one as resolved Nice :) *** This bug has been marked as a duplicate of bug 151026 ***
(In reply to comment #22) > This was landed in a separate bug > (https://bugs.webkit.org/show_bug.cgi?id=151026) > > I don't have editbugs on webkit, but feel free to mark this one as resolved Hi caitp, we should be able to get this bit set for you tomorrow.