Bug 121930 - Implement prefixed-destructuring assignment
Summary: Implement prefixed-destructuring assignment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on: 121981 121999 122202
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-25 16:19 PDT by Oliver Hunt
Modified: 2013-10-10 09:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (88.41 KB, patch)
2013-09-25 16:24 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (88.00 KB, patch)
2013-09-25 16:50 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
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 Details
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 Details
Patch (89.84 KB, patch)
2013-09-25 19:00 PDT, Oliver Hunt
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-09-25 16:19:11 PDT
Implement prefixed-destructuring assignment
Comment 1 Oliver Hunt 2013-09-25 16:24:37 PDT
Created attachment 212636 [details]
Patch
Comment 2 Early Warning System Bot 2013-09-25 16:34:22 PDT
Comment on attachment 212636 [details]
Patch

Attachment 212636 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2187067
Comment 3 Early Warning System Bot 2013-09-25 16:39:56 PDT
Comment on attachment 212636 [details]
Patch

Attachment 212636 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/2187069
Comment 4 Oliver Hunt 2013-09-25 16:50:57 PDT
Created attachment 212641 [details]
Patch
Comment 5 Early Warning System Bot 2013-09-25 17:06:29 PDT
Comment on attachment 212641 [details]
Patch

Attachment 212641 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2186072
Comment 6 Early Warning System Bot 2013-09-25 17:12:54 PDT
Comment on attachment 212641 [details]
Patch

Attachment 212641 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/2165087
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Oliver Hunt 2013-09-25 19:00:23 PDT
Created attachment 212652 [details]
Patch
Comment 12 Mark Hahnenberg 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.
Comment 13 Oliver Hunt 2013-09-26 09:51:23 PDT
Committed r156464: <http://trac.webkit.org/changeset/156464>
Comment 14 Ralph T 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.
Comment 15 Csaba Osztrogonác 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) ...
Comment 16 Oliver Hunt 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
Comment 17 Ralph T 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.
Comment 18 Ralph T 2013-09-26 14:06:44 PDT
Whoops, I misspoke -- that GCC actually generates some error messages, the two versions I tried just spin.
Comment 19 WebKit Commit Bot 2013-09-26 14:22:25 PDT
Re-opened since this is blocked by bug 121981
Comment 20 Timothy Hatcher 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
Comment 21 Csaba Osztrogonác 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 /
Comment 22 Oliver Hunt 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.
Comment 23 Filip Pizlo 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.
Comment 24 Filip Pizlo 2013-10-01 19:18:46 PDT
Rolled out in http://trac.webkit.org/changeset/156757
Comment 25 Oliver Hunt 2013-10-02 12:09:44 PDT
Committed r156785: <http://trac.webkit.org/changeset/156785>