-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
spread op shouldn't slice arguments #1015
Comments
Just to clarify, this is a performance issue, not a correctness one, right? |
@MatrixFrog correct - performance only. However, I also share the concern. It might be better to fix this with a peephole optimization since this is a common code pattern. It allows us to write the shorter code, but let's the compiler fix it. If ever the performance penalty is removed, we could simply undo the optimization. This has the added benefit of fixing the pattern everywhere instead of simply not using it for transpilation. |
Yeah, I like that approach. Out of curiosity, do we know why this is slow? Is it slow on all JS engines? Is it just because |
I'd also like to understand why the slowdown exists, before changing the transpilation. |
Passing While the article is V8 specific, I believe this is common among engines. |
Thanks for the link. I think we should fix this at transpilation. I don't see the point in introducing slow code just to remove it later. We also want to support people who transpile but don't do optimizations. It adds some more bytes in size, but our general principle has been to make as small as possible without introducing serious performance pitfalls. |
The peephole optimization for calling slice on arguments would likely still be useful but indeed is a separate issues. |
Using the spread op in an ES6 function:
compiles to something like:
This leaks and prevents compiler optimization (There's about a 20x slowdown here). As much as I dislike code bloating, I believe it should compile to:
or similiar (obviously slightly different if the spread isn't the first argument).
The text was updated successfully, but these errors were encountered: