-
Notifications
You must be signed in to change notification settings - Fork 189
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
Apply functions: optimizations #1358
Conversation
Is this draft on purpose ? what's the plan ? |
Also, I'd be very interested in seeing some documentation of this aspect of v8. How do you know |
I would like to do more benchmarking. |
I have not seen any documentation on this. I just noticed that somehow. This was also reported in #1246. |
I pulled this change into our repo and saw a huge improvement for microbenchmarks (50% runtime reduction in some cases) but much smaller (2% - 3%) improvement in our benchmarks that attempt to replicate real-world applications and workloads. I suspect that this is because the fallback case in I suspect that if |
This is because if |
not sure about the performance, but one could use a shorter sytnax
instead of
|
19fcd3e
to
5c63ff4
Compare
|
function caml_call_gen(f, args) {
if(f.fun)
return caml_call_gen(f.fun, args);
//FIXME, can happen with too many arguments
if(typeof f !== "function") return f;
var n = f.length | 0;
... Shouldn't we also use the cache for computing |
I pulled this PR in for prototyping and added @hhugo's suggestion - var n = f.length | 0;
+ var n = (f.l >= 0 ? f.l : f.l = f.length) | 0; And the benchmarks I mentioned
saw improvements in the 10% to 30% range! |
@TyOverby Thanks a lot for your tests! I'm surprised this makes that much of a difference since this is on a slow path. Maybe that's the optimization for partially applied functions (33c3af8) that makes such a difference? |
I totally agree that it's almost all due to returning functions with specialized arity when partially applied. I just wanted to mention that I cherry-picked a change that wasn't already in the PR. |
I traced a few apps and found this distribution for the
Based on this, I think I'm happy with only building specialized partially-applied functions for the the cases where only 1 or 2 args remain. |
@vouillon, should we move this PR forward ? |
f.length is slow with v8, so we cache the function arity in another property
I've rebased the PR, fix tests and rewrote history |
Getting the arity of a function is slow with
v8
. On Chrome, functionp
is about ten times slower than functionq
:node
takes 3.1s to execute this piece of code without this optimization and 1.3s with it (and 0,8s when manually removing the call tocaml_call1
).I have known about this issue for a long time. I was considering wrapping functions inside an object:
{arity:2,fun:function(...){..}}
, with some optimization for when the function is statically known. But that would had been a large change with some impact on the generated code size and of lot of added complexity in theJs_of_ocaml
compiler. This change seems to address the issue at a very low cost.Related issue: #1246