Rename local variable arguments to args

[ghost]

This may be affecting performance? CC @rbuckton

[RyanCavanaugh]

Any measurements?

[yuit]

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

[rbuckton]

That would be a breaking change to API consumers.

[rbuckton]

@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]

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]

@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]

Ah, forgot we're compiling to ES5.