Skip to content
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.

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

leaky arguments - broad conclusion insufficiently supported #23

Open
mncharity opened this issue Aug 14, 2017 · 1 comment
Open

leaky arguments - broad conclusion insufficiently supported #23

mncharity opened this issue Aug 14, 2017 · 1 comment

Comments

@mncharity
Copy link

Further in Node 8.3+ we won't be punished for exposing the arguments object to other functions

This seems too strong a conclusion to be supported by bench/arguments.js's

function leakyArguments () {
  return other(arguments)
}

In the past, f.apply(o, arguments) was special cased. But to avoid arguments arrayification, there was the additional requirement that the call site be monomorphic - that there was no diversity in function shape (normal, strict, bound, arrow, etc).

This one benchmark case explores passing arguments to a named inlineable function.

Any conclusions broader than that seem unsupported. The inlining may be required, or the named-ness, or the monomorphism. And given the historical "fragility" of avoiding arguments arrayification, there may be additional requirements. Examining source, or authoritative guidance, or more extensive tests, seem needed to support the current conclusion.

@mcollina
Copy link
Collaborator

@mncharity thanks for reporting! Would you like to provide some counter examples were arguments is getting slower again?

As a side note: arrow functions do not have arguments, and the usage of apply with mixed normal/strict mode is probably not common. I check the bound case and I can confirm the same behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@mcollina @mncharity and others