-
Notifications
You must be signed in to change notification settings - Fork 188
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
runtime: fix useArrowFunction and noArguments #1682
base: master
Are you sure you want to change the base?
Conversation
var len = Math.min(arguments.length, arity); | ||
for (var i = 0; i < len; i++) args[i] = arguments[i]; | ||
return (...args) => { | ||
args.length = arity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this trick to change the length of an array. I will comment the benchmark later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks like it contains some formatting changes (using arrow functions) and also rewrites some of the most foundational primitives in the jsoo runtime. Could you split these up so that that latter can be thouroughly reviewed and benchmarked? |
Also, why are you interested in using arrow functions and avoid using |
1358f63
to
68b76ed
Compare
Context Differences
The arrow function has no binding processing, so you might see some performance improvement in this case. That said, you should not see much difference in general. |
68b76ed
to
03d50f8
Compare
Does this ever come up in jsoo? |
@TyOverby See |
03d50f8
to
b284944
Compare
|
The rest parameter and arguments show almost no difference in the benchmark results for major browsers: https://jsben.ch/BQEVR |
But Chrome is converting it to an array under the hood too. I benchmarked this a few years ago and found that using the spread operator had no impact on speed |
The rest parameter seems to be faster for many arguments: https://jsben.ch/Krmit |
It's true, but it has a certain advantage here because it simplifies the runtime code. If we are going to introduce const/let, ES6 is required, and if we don't see any performance regression, I think we can consider this kind of change positively. |
On my machine at least, the new code is reliably 4% slower.
I think we should prioritize calling functions that have a small number of arguments, as small-arity functions are more common in OCaml than functions with 100+ parameters.
I modified your benchmark so that the arrays aren't empty, and to do some fast computation with them instead of logging (logging is slow, and mucks with benchmark results), and this new benchmark shows that even for 100-parameter calls, the old method is twice as fast (on Chrome at least) than using the new syntax: https://jsben.ch/sGBgk |
I'm seeing a strange result 🤔 (I'm also on the latest Chrome btw) |
I ran it on the latest Chrome with Apple Silicon. What's your environment? |
Latest Chrome x86-64 |
Signed-off-by: Sora Morimoto <[email protected]>
b284944
to
7ea59a4
Compare
Note: this partially introduces ES6!