The most trivial of cases would be eliminating two array allocations and the array iterator protocol loop in something like this: ``` function foo(...args) { return bar(...args); } ``` ES6SampleBench/Air has code similar to this: ``` visitArg(index, func, ...args) { let replacement = func(this._args[index], ...args); if (replacement) this._args[index] = replacement; } ``` Since this function is never called with more than 3 arguments, I rewrote the code like this to see what the performance improvement ballpark is: ``` visitArg(index, func, a1, a2, a3) { let replacement = func(this._args[index], a1, a2, a3); if (replacement) this._args[index] = replacement; } ``` This made the benchmark speed up by ~50%. I'm guessing we should be able to make this benchmark at least 30-45% faster by optimizing this type of pattern in the DFG/FTL.
Created attachment 294622 [details] WIP It beings. It can run this program and do the proper optimization: ``` function assert(b) { if (!b) throw new Error; } function baz(a, b, c, d) { return [a, b, c, d]; } function bar(a, b, ...args) { return baz(a, b, ...args); } function foo(a, b, c, d) { return bar(a, b, c, d); } noInline(foo); for (let i = 0; i < 10000; i++) { let [a, b, c, d] = foo(i, i+1, i+2, i+3); assert(a === i); assert(b === i+1); assert(c === i+2); assert(d === i+3); } ```
Created attachment 294736 [details] WIP Some more things, it now kind of works with a new type of ForwardVarargs.
Created attachment 294911 [details] WIP It might be able to run SampleBench/Air now. Still have quite a few things left.
Created attachment 294996 [details] WIP We can forward varargs from a spread, and we can call varargs with spread. I still need to do OSR exit and make sure all the things work. I also want to make NewArrayWithSpread understand how to create itself from PhantomSpreads even if the NewArrayWithSpread itself is escaped.
Created attachment 295016 [details] WIP OSR exit is starting to work.
Created attachment 295037 [details] WIP NewArrayWithSpread now can accept arbitrary PhantomSpreads as arguments even if it itself must be allocated. PhantomSpread can only become Phantom if its argument is CreateRest and the CreateRest doesn't escape. I think this is probably needed for correctness because we don't know if any other incoming array has arbitrary indexed getters on it. If we want to PhantomSpread arbitrary arrays, we probably need a way to prove that the act of spreading wouldn't call into user observable code.
Created attachment 295122 [details] WIP passes almost all JSC tests. Just need to write some more specific testing for this.
Created attachment 295319 [details] patch I think this patch is just about ready, I just need to write a changelog and a few more tests.
Attachment 295319 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:994: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 295476 [details] patch
Attachment 295476 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:19: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:49: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:50: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:67: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:68: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:69: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:979: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 8 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
*** Bug 164806 has been marked as a duplicate of this bug. ***
Comment on attachment 295476 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295476&action=review > Source/JavaScriptCore/b3/B3ValueRep.h:81 > + // It's not valid output representation. *a valid > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4331 > if (m_graph.isWatchingHavingABadTimeWatchpoint(m_node)) { I don't know from this method name if it means that we are having a bad time or if the watchpoint set is valid (i.e. we are not having a bad time).
(In reply to comment #13) > Comment on attachment 295476 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295476&action=review > > > Source/JavaScriptCore/b3/B3ValueRep.h:81 > > + // It's not valid output representation. > > *a valid > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4331 > > if (m_graph.isWatchingHavingABadTimeWatchpoint(m_node)) { > > I don't know from this method name if it means that we are having a bad time > or if the watchpoint set is valid (i.e. we are not having a bad time). Never mind, I was reading this wrong. I didn't parse the "isWatching" right. Please disregard.
Created attachment 295693 [details] patch for landing
Created attachment 295694 [details] patch for landing
Comment on attachment 295694 [details] patch for landing Clearing flags on attachment: 295694 Committed r209121: <http://trac.webkit.org/changeset/209121>
All reviewed patches have been landed. Closing bug.