Skip to content

Rename local variable arguments to args#14996

Merged
1 commit merged into
masterfrom
args
Apr 5, 2017
Merged

Rename local variable arguments to args#14996
1 commit merged into
masterfrom
args

Conversation

@ghost

@ghost ghost commented Apr 3, 2017

Copy link
Copy Markdown

This may be affecting performance? CC @rbuckton

@RyanCavanaugh

Copy link
Copy Markdown
Member

Any measurements?

Comment thread src/compiler/utilities.ts
return false;
}
const { expression, arguments } = callExpression as CallExpression;
const { expression, arguments: args } = callExpression as CallExpression;

@yuit yuit Apr 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be better to just rename property in CallExpression? Also would this change should be applied to newExpression as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That would be a breaking change to API consumers.

@rbuckton

rbuckton commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

@RyanCavanaugh there was a spike in the build-to-build performance tests associated with this change. Whether this was the direct cause I haven't yet determined. Regardless of the performance metrics, we should never shadow arguments with a local or reassign it. It deoptimizes the function in v8, and is an error in strict mode.

@ghost

ghost commented Apr 5, 2017

Copy link
Copy Markdown
Author

The PR that added this code was #14984 on April 3rd. The performance tester showed emit time spike after that commit, but this function is called by the binder and checker. In the time since, emit time has gone back down.
That link doesn't mention shadowing, just mutation. Can you verify that deoptimization is actually taking place?

@rbuckton

rbuckton commented Apr 5, 2017

Copy link
Copy Markdown
Contributor

@Andy-MS the initial arguments binding is hoisted inside of a function. A "redeclaration" of arguments using var doesn't really do anything, as can be observed in node:

> function f() { var arguments; console.log(arguments.length); }
> f(); 
0
> f("a");
1

So any assignment to arguments, even with a variable initializer, is actually reassigning the initial arguments binding.

That said, I ran a javascript project through tsc with hydrogen tracing and didn't see any specific deoptimizations in this function. I still think its worth the change.

@ghost

ghost commented Apr 5, 2017

Copy link
Copy Markdown
Author

Ah, forgot we're compiling to ES5.

@ghost ghost merged commit bb8862f into master Apr 5, 2017
@ghost ghost deleted the args branch April 5, 2017 20:45
@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants