RESOLVED FIXED 121930
Implement prefixed-destructuring assignment
https://bugs.webkit.org/show_bug.cgi?id=121930
Summary Implement prefixed-destructuring assignment
Oliver Hunt
Reported 2013-09-25 16:19:11 PDT
Implement prefixed-destructuring assignment
Attachments
Patch (88.41 KB, patch)
2013-09-25 16:24 PDT, Oliver Hunt
no flags
Patch (88.00 KB, patch)
2013-09-25 16:50 PDT, Oliver Hunt
no flags
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (534.15 KB, application/zip)
2013-09-25 17:40 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (662.11 KB, application/zip)
2013-09-25 18:11 PDT, Build Bot
no flags
Patch (89.84 KB, patch)
2013-09-25 19:00 PDT, Oliver Hunt
mhahnenberg: review+
Oliver Hunt
Comment 1 2013-09-25 16:24:37 PDT
Early Warning System Bot
Comment 2 2013-09-25 16:34:22 PDT
Early Warning System Bot
Comment 3 2013-09-25 16:39:56 PDT
Oliver Hunt
Comment 4 2013-09-25 16:50:57 PDT
Early Warning System Bot
Comment 5 2013-09-25 17:06:29 PDT
Early Warning System Bot
Comment 6 2013-09-25 17:12:54 PDT
Build Bot
Comment 7 2013-09-25 17:40:37 PDT
Comment on attachment 212641 [details] Patch Attachment 212641 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2286067 New failing tests: sputnik/Conformance/13_Function_Definition/S13_A5.html
Build Bot
Comment 8 2013-09-25 17:40:39 PDT
Created attachment 212643 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Build Bot
Comment 9 2013-09-25 18:11:19 PDT
Comment on attachment 212641 [details] Patch Attachment 212641 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2283077 New failing tests: sputnik/Conformance/13_Function_Definition/S13_A5.html
Build Bot
Comment 10 2013-09-25 18:11:21 PDT
Created attachment 212647 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Oliver Hunt
Comment 11 2013-09-25 19:00:23 PDT
Mark Hahnenberg
Comment 12 2013-09-26 09:30:14 PDT
Comment on attachment 212652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212652&action=review r=me > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:398 > } Side note: the BytecodeGenerator constructor seems like it does waaay too much stuff. Maybe we should factor some of this functionality out into separate helper functions? > Source/JavaScriptCore/parser/ASTBuilder.h:322 > + ParameterNode* createFormalParameterList(DeconstructionPattern pattern) { return new (m_vm) ParameterNode(pattern); } > + ParameterNode* createFormalParameterList(ParameterNode* list, DeconstructionPattern pattern) { return new (m_vm) ParameterNode(list, pattern); } It's sad that we can't do PassRefPtrs here, but hopefully move semantics will save us.
Oliver Hunt
Comment 13 2013-09-26 09:51:23 PDT
Ralph T
Comment 14 2013-09-26 12:16:49 PDT
GCC 4.7.3 and 4.8.1 can no longer compile Parser.cpp on linux x86-64 with this change applied.
Csaba Osztrogonác
Comment 15 2013-09-26 12:46:15 PDT
(In reply to comment #14) > GCC 4.7.3 and 4.8.1 can no longer compile Parser.cpp on linux x86-64 with this change applied. Who cares? Apple doesn't use GCC (and EWS) ...
Oliver Hunt
Comment 16 2013-09-26 13:48:12 PDT
(In reply to comment #15) > (In reply to comment #14) > > GCC 4.7.3 and 4.8.1 can no longer compile Parser.cpp on linux x86-64 with this change applied. > Who cares? > Apple doesn't use GCC (and EWS) ... Wow, way to be helpful Ossy, that patch had been up for the EWS bots all night and they hadn't run it. I thought i had fixed the problems that caused the earlier failures. Although I don't understand why there are Qt bots still running (or why the Qt port is even still in tree) given the stated intention to no longer use webkit. (In reply to comment #14) > GCC 4.7.3 and 4.8.1 can no longer compile Parser.cpp on linux x86-64 with this change applied. I fixed one issue that was causing MSVC to have problems compiling, what are the failures you're seeing in GCC? There shouldn't be anything in there that isn't part of the C++11 subset that we now require as part of webkit but it's possible I used something unintentionally
Ralph T
Comment 17 2013-09-26 13:57:20 PDT
(In reply to comment #16) > (In reply to comment #14) > > GCC 4.7.3 and 4.8.1 can no longer compile Parser.cpp on linux x86-64 with this change applied. > > I fixed one issue that was causing MSVC to have problems compiling, what are the failures you're seeing in GCC? There shouldn't be anything in there that isn't part of the C++11 subset that we now require as part of webkit but it's possible I used something unintentionally GCC doesn't terminate and gradually consumes more and more memory. I don't know exactly what's going on inside. You can see the output on any of the GCC bots, for example: https://webkit-queues.appspot.com/results/2574002 (it terminated GCC on Parser.cpp). The change is pretty big and I'm unfamiliar with this part of WebKit; let me know if there's anything I can help test.
Ralph T
Comment 18 2013-09-26 14:06:44 PDT
Whoops, I misspoke -- that GCC actually generates some error messages, the two versions I tried just spin.
WebKit Commit Bot
Comment 19 2013-09-26 14:22:25 PDT
Re-opened since this is blocked by bug 121981
Timothy Hatcher
Comment 20 2013-09-26 14:25:03 PDT
This caused a massive leak that was killing the buildbot. All the leaks are BindingNodes. http://build.webkit.org/builders/Apple%20MountainLion%20%28Leaks%29/builds/7094/steps/layout-test/logs/stdio Leak: 0x7fb2d14d13f0 size=64 zone: DefaultMallocZone_0x10f090000 JSC::BindingNode C++ JavaScriptCore 0x0f9c43e0 0x00000001 0x00000001 0x60000000 .C.............` 0x0000000d 0x0000001e 0x00000001 0x0000000d ................ 0x0000001b 0x00000001 0x0000000d 0x0000001e ................ 0x00000001 0x00007fb2 0xd2023800 0x00007fb2 .........8...... Call stack: [thread 0x7fff77210180]: | 0x2 | start | main DumpRenderTree.mm:955 | dumpRenderTree(int, char const**) DumpRenderTree.mm:917 | runTestingServerLoop() DumpRenderTree.mm:862 | runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) DumpRenderTree.mm:1411 | CFRunLoopRunSpecific | __CFRunLoopRun | __CFRunLoopDoSources0 | __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ | MultiplexerSource::perform() | RunloopBlockContext::perform() | CFArrayApplyFunction | __block_global_1 | ___withDelegateAsync_block_invoke_0 | ___delegate_didFinishLoading_block_invoke_0 | -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] | -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] | __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 | -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] WebCoreResourceHandleAsDelegate.mm:234 | WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) ResourceLoader.cpp:490 | WebCore::SubresourceLoader::didFinishLoading(double) SubresourceLoader.cpp:285 | WebCore::CachedRawResource::finishLoading(WebCore::ResourceBuffer*) CachedRawResource.cpp:95 | WebCore::CachedResource::finishLoading(WebCore::ResourceBuffer*) CachedResource.cpp:386 | WebCore::CachedResource::checkNotify() CachedResource.cpp:369 | WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource*) DocumentLoader.cpp:346 | WebCore::DocumentLoader::finishedLoading(double) DocumentLoader.cpp:409 | WebCore::DocumentWriter::end() DocumentWriter.cpp:243 | WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter*) DecodedDataDocumentParser.cpp:60 | WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) HTMLDocumentParser.cpp:744 | WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) HTMLDocumentParser.cpp:237 | WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) HTMLDocumentParser.cpp:536 | WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) HTMLDocumentParser.cpp:292 | WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() HTMLDocumentParser.cpp:272 | WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) HTMLScriptRunner.cpp:183 | WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) HTMLScriptRunner.cpp:314 | WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) ScriptElement.cpp:246 | WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) ScriptElement.cpp:315 | WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) ScriptController.cpp:158 | WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) ScriptController.cpp:142 | WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) JSMainThreadExecState.h:62 | JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) Completion.cpp:83 | JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) Interpreter.cpp:854 | JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*) Executable.cpp:425 | JSC::JSGlobalObject::createProgramCodeBlock(JSC::ExecState*, JSC::ProgramExecutable*, JSC::JSObject**) JSGlobalObject.cpp:723 | JSC::CodeCache::getProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictness, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&) CodeCache.cpp:120 | JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictness, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&) CodeCache.cpp:92 | WTF::PassRefPtr<JSC::ProgramNode> JSC::parse<JSC::ProgramNode>(JSC::VM*, JSC::SourceCode const&, JSC::FunctionParameters*, JSC::Identifier const&, JSC::JSParserStrictness, JSC::JSParserMode, JSC::ParserError&) Parser.h:1118 | WTF::PassRefPtr<JSC::ProgramNode> JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::ProgramNode>(JSC::ParserError&) Parser.h:1052 | JSC::Parser<JSC::Lexer<unsigned char> >::parseInner() Parser.cpp:120 | JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<(JSC::SourceElementsMode)0, JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:176 | JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) Parser.cpp:886 | JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionOrLabelStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1070 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1218 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1245 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1304 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1343 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1827 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1765 | JSC::ASTBuilder::Arguments JSC::Parser<JSC::Lexer<unsigned char> >::parseArguments<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1694 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1245 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1304 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1343 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1827 | JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:1732 | bool JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionInfo<(JSC::FunctionRequirements)0, false, JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, JSC::ASTBuilder::FormalParameterList&, JSC::ASTBuilder::FunctionBody&, unsigned int&, unsigned int&, int&, unsigned int&) Parser.cpp:946 | JSC::ASTBuilder::FormalParameterList JSC::Parser<JSC::Lexer<unsigned char> >::parseFormalParameters<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:908 | JSC::ASTBuilder::DeconstructionPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDeconstructionPattern<(JSC::Parser<JSC::Lexer<unsigned char> >::DeconstructionKind)1, JSC::ASTBuilder>(JSC::ASTBuilder&, int) Parser.cpp:431 | JSC::ASTBuilder::DeconstructionPattern JSC::Parser<JSC::Lexer<unsigned char> >::createBindingPattern<(JSC::Parser<JSC::Lexer<unsigned char> >::DeconstructionKind)1, JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const&, int) Parser.cpp:360 | JSC::ASTBuilder::createBindingLocation(JSC::JSTokenLocation const&, JSC::Identifier const&, JSC::JSTextPosition const&, JSC::JSTextPosition const&, JSC::JSTextPosition const&) ASTBuilder.h:655 | JSC::BindingNode::create(JSC::VM*, JSC::Identifier const&, JSC::JSTextPosition const&, JSC::JSTextPosition const&, JSC::JSTextPosition const&) NodeConstructors.h:858 | JSC::DeconstructionPatternNode::operator new(unsigned long) Nodes.h:1539 | WTF::fastMalloc(unsigned long) FastMalloc.cpp:287 | malloc | malloc_zone_malloc
Csaba Osztrogonác
Comment 21 2013-09-27 01:50:42 PDT
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > GCC 4.7.3 and 4.8.1 can no longer compile Parser.cpp on linux x86-64 with this change applied. > > Who cares? > > Apple doesn't use GCC (and EWS) ... > > Wow, way to be helpful Ossy, that patch had been up for the EWS bots all night and they hadn't run it. I thought i had fixed the problems that caused the earlier failures. If you had clicked on the purple bubbles, you would have seen that EWS bots can't apply the patch, that's why they don't run. ( Otherwise watching the bots after landing isn't forbidden. :) ) And because of this breakage, one more breakage landed in the tree, because EWS don't run .. - https://bugs.webkit.org/show_bug.cgi?id=121983 > Although I don't understand why there are Qt bots still running (or why the Qt port is even still in tree) given the stated intention to no longer use webkit. How is it related to me or this bug? I don't work on Qt port, I am not the Qt port and Qt port isn't me. If you would like to know more about the future of the Qt port, I suggest you should follow this thread on webkit-dev mailing list: Changes in QtWebKit development. ( Otherwise Qt buildbots and EWS bots are still the fastest bots and they notice very quickly if something goes wrong. ) Just out of curiosity if the contributing rules are burden for you, why don't you change "all platforms" to "Apple Mac" by fiat? "Keeping the tree green ... Your change must at least compile on all platforms. ..." / http://www.webkit.org/coding/contributing.html /
Oliver Hunt
Comment 22 2013-09-27 10:46:11 PDT
> How is it related to me or this bug? I don't work on Qt port, I am not the Qt > port and Qt port isn't me. If you would like to know more about the future of > the Qt port, I suggest you should follow this thread on webkit-dev mailing > list: Changes in QtWebKit development. Okay, in that case you don't actually do any work on webkit, you just make unhelpful comments in bugzilla. People other than you actually worked to help me diagnose the problem with the gcc build. By actually contributing other people helped webkit get better. On the other hand, your comments did nothing to help anyone.
Filip Pizlo
Comment 23 2013-10-01 18:59:04 PDT
This regressed string-fasta by 50%. I'm going to roll it out. Please run performance tests next time you make such large changes. It's very painful to have to find the performance regressions so long after the fact.
Filip Pizlo
Comment 24 2013-10-01 19:18:46 PDT
Oliver Hunt
Comment 25 2013-10-02 12:09:44 PDT
Note You need to log in before you can comment on or make changes to this bug.