Skip to content

Optimize rest parameter emit #20307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Optimize rest parameter emit #20307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8afbf8b
Optimize rest parameter emit #19182
charlespierce Nov 28, 2017
7b4d920
Merge branch 'master' into rest_parameter_optimization
charlespierce Jan 20, 2018
d7955ab
Update baselines for new tests
charlespierce Jan 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 52 additions & 13 deletions src/compiler/transformers/es2015.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,7 +944,6 @@ namespace ts {

if (constructor) {
addDefaultValueAssignmentsIfNeeded(statements, constructor);
addRestParameterIfNeeded(statements, constructor, hasSynthesizedSuper);
if (!hasSynthesizedSuper) {
// If no super call has been synthesized, emit custom prologue directives.
statementOffset = addCustomPrologue(statements, constructor.body.statements, statementOffset, visitor);
Expand All @@ -969,6 +968,7 @@ namespace ts {
}

addRange(statements, visitNodes(constructor.body.statements, visitor, isStatement, /*start*/ statementOffset));
insertRestParameterIfNeeded(statements, constructor, hasSynthesizedSuper);
}

// Return `_this` unless we're sure enough that it would be pointless to add a return statement.
Expand Down Expand Up @@ -1344,6 +1344,38 @@ namespace ts {
return node && node.dotDotDotToken && node.name.kind === SyntaxKind.Identifier && !inConstructorWithSynthesizedSuper;
}

/**
* Gets the index of the first statement that contains a reference to a given parameter
*
* @param restParameter The ParameterDeclaration to which we are finding references.
* @param statements An array of Statement nodes to search
*/
function findRestParameterReferenceIndex(restParameter: ParameterDeclaration, statements: Statement[]) {
const originalRestParameter = getOriginalNode(restParameter);

return findIndex(statements, containsReferenceToParameter);

function containsReferenceToParameter(statement: Statement): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in a recursive walk of the entire subtree of every statement of a function or method, which we try to avoid in the transforms. It seems like this is best handled in the checker when checking an Identifier. At that point you can look up its declaration, determine whether the declaration is a rest parameter, and walk up from there to the topmost containing statement and set something like a NodeCheckFlags.ContainsRestParameterReference on the statement. Walking up the spine is far cheaper than walking down to each descendant.

Note that you would also need to verify here in the transformer that the rest parameter was type checked, as its possible that a transformation added a previously non-existent rest-parameter. In that case, I would err on the side of caution and emit at the top of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I wasn't aware of trying to avoid recursive walks in transforms. I'll take a look at flagging this info (and the below walk as well) during type checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a tree walk is generally cheap, each transform is run in succession resulting in multiple tree walks per file. In the compiler we attempt to limit this as much as possible through various optimizations. One optimization we regularly employ (though it doesn't apply here) are transformation flags that we calculate during our bind step (since bind is a guaranteed top-down full walk of the tree), or check flags that we calculate in the checker (which can randomly access the tree). These flags allow us to avoid full walks of the tree because we can use that information to determine whether a tree walk is necessary for each case.

return (isIdentifier(statement) && originalRestParameter === resolver.getReferencedValueDeclaration(statement)) ||
forEachChild(statement, containsReferenceToParameter);
}
}

/**
* Gets the index of the first statement that contains a re-definition of "arguments"
*
* @param statements An array of Statement nodes to search
*/
function findArgumentsRedefinitionIndex(statements: Statement[]) {
return findIndex(statements, containsArgumentsRedeclaration);

function containsArgumentsRedeclaration(statement: Statement): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this is yet another expensive recursive walk of the entire subtree and is better handled in the checker.

return (isAssignmentExpression(statement) && isIdentifier(statement.left) && statement.left.escapedText === "arguments") ||
(isVariableDeclaration(statement) && statement.initializer && (<Identifier>statement.name).escapedText === "arguments") ||
forEachChild(statement, containsArgumentsRedeclaration);
}
}

/**
* Adds statements to the body of a function-like node if it contains a rest parameter.
*
Expand All @@ -1353,12 +1385,23 @@ namespace ts {
* part of a constructor declaration with a
* synthesized call to `super`
*/
function addRestParameterIfNeeded(statements: Statement[], node: FunctionLikeDeclaration, inConstructorWithSynthesizedSuper: boolean): void {
function insertRestParameterIfNeeded(statements: Statement[], node: FunctionLikeDeclaration, inConstructorWithSynthesizedSuper: boolean): void {
const parameter = lastOrUndefined(node.parameters);
if (!shouldAddRestParameter(parameter, inConstructorWithSynthesizedSuper)) {
return;
}

let referenceIndex = findRestParameterReferenceIndex(parameter, statements);
if (referenceIndex === -1) {
return;
}

const argumentsRedefinitionIndex = findArgumentsRedefinitionIndex(statements);
if (argumentsRedefinitionIndex > -1) {
// If the "arguments" variable is redefined, need to make sure to emit the rest parameter initialization before then.
referenceIndex = Math.min(referenceIndex, argumentsRedefinitionIndex);
}

// `declarationName` is the name of the local declaration for the parameter.
const declarationName = getMutableClone(<Identifier>parameter.name);
setEmitFlags(declarationName, EmitFlags.NoSourceMap);
Expand All @@ -1369,9 +1412,7 @@ namespace ts {
const temp = createLoopVariable();

// var param = [];
statements.push(
setEmitFlags(
setTextRange(
const variableStatement = setTextRange(
createVariableStatement(
/*modifiers*/ undefined,
createVariableDeclarationList([
Expand All @@ -1383,10 +1424,7 @@ namespace ts {
])
),
/*location*/ parameter
),
EmitFlags.CustomPrologue
)
);
);

// for (var _i = restIndex; _i < arguments.length; _i++) {
// param[_i - restIndex] = arguments[_i];
Expand Down Expand Up @@ -1426,9 +1464,8 @@ namespace ts {
])
);

setEmitFlags(forStatement, EmitFlags.CustomPrologue);
startOnNewLine(forStatement);
statements.push(forStatement);
statements.splice(referenceIndex, 0, variableStatement, forStatement);
}

/**
Expand Down Expand Up @@ -1846,7 +1883,6 @@ namespace ts {

addCaptureThisForNodeIfNeeded(statements, node);
addDefaultValueAssignmentsIfNeeded(statements, node);
addRestParameterIfNeeded(statements, node, /*inConstructorWithSynthesizedSuper*/ false);

// If we added any generated statements, this must be a multi-line block.
if (!multiLine && statements.length > 0) {
Expand Down Expand Up @@ -1895,13 +1931,16 @@ namespace ts {
closeBraceLocation = body;
}

const statementBodyLength = statements.length;
insertRestParameterIfNeeded(statements, node, /*inConstructorWithSynthesizedSuper*/ false);

const lexicalEnvironment = context.endLexicalEnvironment();
addRange(statements, lexicalEnvironment);

prependCaptureNewTargetIfNeeded(statements, node, /*copyOnWrite*/ false);

// If we added any final generated statements, this must be a multi-line block
if (!multiLine && lexicalEnvironment && lexicalEnvironment.length) {
if (!multiLine && statements.length !== statementBodyLength) {
multiLine = true;
}

Expand Down
14 changes: 2 additions & 12 deletions tests/baselines/reference/accessorWithRestParam.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,12 @@ var C = /** @class */ (function () {
function C() {
}
Object.defineProperty(C.prototype, "X", {
set: function () {
var v = [];
for (var _i = 0; _i < arguments.length; _i++) {
v[_i] = arguments[_i];
}
},
set: function () { },
enumerable: true,
configurable: true
});
Object.defineProperty(C, "X", {
set: function () {
var v2 = [];
for (var _i = 0; _i < arguments.length; _i++) {
v2[_i] = arguments[_i];
}
},
set: function () { },
enumerable: true,
configurable: true
});
Expand Down
7 changes: 1 addition & 6 deletions tests/baselines/reference/arrayLiteralInNonVarArgParameter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,5 @@ panic([], 'one', 'two');


//// [arrayLiteralInNonVarArgParameter.js]
function panic(val) {
var opt = [];
for (var _i = 1; _i < arguments.length; _i++) {
opt[_i - 1] = arguments[_i];
}
}
function panic(val) { }
panic([], 'one', 'two');
56 changes: 7 additions & 49 deletions tests/baselines/reference/assignmentCompatWithCallSignaturesWithRestParameters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,49 +49,19 @@ var a4: (x?: number, y?: string, ...z: number[]) => number;
// call signatures in derived types must have the same or fewer optional parameters as the target for assignment
var a; // ok, same number of required params
a = function () { return 1; }; // ok, same number of required params
a = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
return 1;
}; // ok, same number of required params
a = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
return 1;
}; // error, type mismatch
a = function () { return 1; }; // ok, same number of required params
a = function () { return 1; }; // error, type mismatch
a = function (x) { return 1; }; // ok, same number of required params
a = function (x, y, z) { return 1; }; // ok, same number of required params
a = function (x) { return 1; }; // ok, rest param corresponds to infinite number of params
a = function (x) { return 1; }; // error, incompatible type
var a2;
a2 = function () { return 1; }; // ok, fewer required params
a2 = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
return 1;
}; // ok, fewer required params
a2 = function () { return 1; }; // ok, fewer required params
a2 = function (x) { return 1; }; // ok, fewer required params
a2 = function (x) { return 1; }; // ok, same number of required params
a2 = function (x) {
var args = [];
for (var _i = 1; _i < arguments.length; _i++) {
args[_i - 1] = arguments[_i];
}
return 1;
}; // ok, same number of required params
a2 = function (x) {
var args = [];
for (var _i = 1; _i < arguments.length; _i++) {
args[_i - 1] = arguments[_i];
}
return 1;
}; // should be type mismatch error
a2 = function (x) { return 1; }; // ok, same number of required params
a2 = function (x) { return 1; }; // should be type mismatch error
a2 = function (x, y) { return 1; }; // ok, rest param corresponds to infinite number of params
a2 = function (x, y) { return 1; }; // ok, same number of required params
var a3;
Expand All @@ -100,24 +70,12 @@ a3 = function (x) { return 1; }; // ok, fewer required params
a3 = function (x) { return 1; }; // ok, same number of required params
a3 = function (x, y) { return 1; }; // ok, all present params match
a3 = function (x, y, z) { return 1; }; // error
a3 = function (x) {
var z = [];
for (var _i = 1; _i < arguments.length; _i++) {
z[_i - 1] = arguments[_i];
}
return 1;
}; // error
a3 = function (x) { return 1; }; // error
a3 = function (x, y, z) { return 1; }; // error
var a4;
a4 = function () { return 1; }; // ok, fewer required params
a4 = function (x, y) { return 1; }; // error, type mismatch
a4 = function (x) { return 1; }; // ok, all present params match
a4 = function (x, y) { return 1; }; // error, second param has type mismatch
a4 = function (x, y) { return 1; }; // ok, same number of required params with matching types
a4 = function (x) {
var args = [];
for (var _i = 1; _i < arguments.length; _i++) {
args[_i - 1] = arguments[_i];
}
return 1;
}; // error, rest params have type mismatch
a4 = function (x) { return 1; }; // error, rest params have type mismatch
7 changes: 1 addition & 6 deletions tests/baselines/reference/baseTypeAfterDerivedType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ interface Base2 {
var Derived2 = /** @class */ (function () {
function Derived2() {
}
Derived2.prototype.method = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
};
Derived2.prototype.method = function () { };
return Derived2;
}());
10 changes: 1 addition & 9 deletions tests/baselines/reference/callWithSpread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ var __extends = (this && this.__extends) || (function () {
};
})();
function foo(x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
}
var a;
var z;
Expand All @@ -100,18 +96,14 @@ xa[1].foo(1, 2, "abc");
(_g = xa[1]).foo.apply(_g, [1, 2, "abc"]);
var C = /** @class */ (function () {
function C(x, y) {
this.foo(x, y);
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
this.foo(x, y);
this.foo.apply(this, [x, y].concat(z));
}
C.prototype.foo = function (x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
};
return C;
}());
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/capturedLetConstInLoop13.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,17 @@ var Main = /** @class */ (function () {
}
Main.prototype.register = function () {
var _this = this;
var names = [];
for (var _i = 0; _i < arguments.length; _i++) {
names[_i] = arguments[_i];
}
var _loop_1 = function (name) {
this_1.bar((_a = {},
_a[name + ".a"] = function () { _this.foo(name); },
_a));
var _a;
};
var this_1 = this;
var names = [];
for (var _i = 0; _i < arguments.length; _i++) {
names[_i] = arguments[_i];
}
for (var _a = 0, names_1 = names; _a < names_1.length; _a++) {
var name = names_1[_a];
_loop_1(name);
Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/checkSuperCallBeforeThisAccessing5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ var __extends = (this && this.__extends) || (function () {
})();
var Based = /** @class */ (function () {
function Based() {
var arg = [];
for (var _i = 0; _i < arguments.length; _i++) {
arg[_i] = arguments[_i];
}
}
return Based;
}());
Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/checkSuperCallBeforeThisAccessing6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ var __extends = (this && this.__extends) || (function () {
})();
var Base = /** @class */ (function () {
function Base() {
var arg = [];
for (var _i = 0; _i < arguments.length; _i++) {
arg[_i] = arguments[_i];
}
}
return Base;
}());
Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/checkSuperCallBeforeThisAccessing8.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ var __extends = (this && this.__extends) || (function () {
})();
var Base = /** @class */ (function () {
function Base() {
var arg = [];
for (var _i = 0; _i < arguments.length; _i++) {
arg[_i] = arguments[_i];
}
}
return Base;
}());
Expand Down
8 changes: 0 additions & 8 deletions tests/baselines/reference/collisionArgumentsArrowFunctions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,12 @@ var f1 = function (i) {
var arguments; // no error
};
var f12 = function (arguments) {
var rest = [];
for (var _i = 1; _i < arguments.length; _i++) {
rest[_i - 1] = arguments[_i];
}
var arguments = 10; // no error
};
var f1NoError = function (arguments) {
var arguments = 10; // no error
};
var f2 = function () {
var restParameters = [];
for (var _i = 0; _i < arguments.length; _i++) {
restParameters[_i] = arguments[_i];
}
var arguments = 10; // No Error
};
var f2NoError = function () {
Expand Down
Loading