ocsigen
runtime: fix useArrowFunction and noArguments
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 arguments?
Context Differences
- The Arrow function inherits this from its parent. This may reduce the overhead of binding this by using the Arrow function.
- The function function binds this to its own scope, so you must explicitly bind this to prevent unintended behaviour in situations where this is treated differently.
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.
The function function binds this to its own scope, so you must explicitly bind this to prevent unintended behaviour in situations where this is treated differently.
Does this ever come up in jsoo?
Also, why are you interested in using arrow functions and avoid using
arguments?
arguments is not Array, it's ArrayLike, and converting it to Array has some cost. Also, newer JS engines have better optimisation for the rest parameter.
The rest parameter and arguments show almost no difference in the benchmark results for major browsers: https://jsben.ch/BQEVR
arguments is not Array, it's ArrayLike, and converting it to Array has some cost
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
arguments is not Array, it's ArrayLike, and converting it to Array has some cost
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
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.
The rest parameter and arguments show almost no difference in the benchmark results for major browsers: https://jsben.ch/BQEVR
On my machine at least, the new code is reliably 4% slower.
The rest parameter seems to be faster for many arguments: https://jsben.ch/Krmit
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.
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.
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 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?
I ran it on the latest Chrome with Apple Silicon. What's your environment?
Latest Chrome x86-64
I tested it again and it's still fast in all environments. I saw that it was very slow when ads loaded, so that might be the cause?
Chrome:
Firefox:
Safari:
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?
@smorimoto, I think it would be good to split the two changes as suggested above so that they can be tested independently.
To fix the noArguments, you need the arrow function, and to fix the useArrowFunction, you need to remove the arguments...
To fix the noArguments, you need the arrow function, and to fix the useArrowFunction, you need to remove the arguments...
I don't see the relation, sorry. I would expect that only spread is needed to replace usage of arguments.
After asking for the PR to be split up, I did the benchmarking which has me pretty confident that the change should instead be abandoned, so I don’t want anyone doing the extra work of splitting the feature up if it’s not going to be merged.
After asking for the PR to be split up, I did the benchmarking which has me pretty confident that the change should instead be abandoned, so I don’t want anyone doing the extra work of splitting the feature up if it’s not going to be merged.
Which part is slower ?
@TyOverby, Have you run more that just micro benchmarks ? Were you able to see differences with real applications ?
I haven’t run this on any real applications because we aren’t easily able to pull in jsoo branches until wasm is merged. However, i think that microbenchmarks are sufficient to reject what is effectively a code formatting change
I haven’t run this on any real applications because we aren’t easily able to pull in jsoo branches until wasm is merged. However, i think that microbenchmarks are sufficient to reject what is effectively a code formatting change
Except this PR is not only about formatting. Also, I've been tricked many times by javascript micro benchmarks because real program would sometime show opposite result. The last one was https://github.com/ocsigen/js_of_ocaml/pull/1647 that I ended up reverting.
What is the purpose of the PR then? Did some user of Js_of_ocaml need the runtime javascript to be more “modern” for some reason?
FWIW, I find the new style of code in this PR to be harder to read than the original, so without a good reason to do it (like performance), i’m pretty opposed
FWIW, I find the new style of code in this PR to be harder to read than the original, so without a good reason to do it (like performance), i’m pretty opposed
This PR will not be merge in its current state (You and I both suggested to split it up). I've extracted and reworked part of it in https://github.com/ocsigen/js_of_ocaml/pull/1740 and I personally find the code much more digest in its new form.
Benchmarking with CAMLBOY is probably useful: https://github.com/linoscope/CAMLBOY
5.8.2
First Attempt
ROM path: ./tobu.gb
Frames: 5000
Duration: 3.127700
FPS: 1598.618793
Second attempt
ROM path: ./tobu.gb
Frames: 5000
Duration: 2.946000
FPS: 1697.216565
This PR
First Attempt
ROM path: ./tobu.gb
Frames: 5000
Duration: 2.972200
FPS: 1682.255568
Second attempt
ROM path: ./tobu.gb
Frames: 5000
Duration: 2.964000
FPS: 1686.909582