runtime: fix useArrowFunction and noArguments

[smorimoto]

Note: this partially introduces ES6!

[TyOverby]

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?

[TyOverby]

Also, why are you interested in using arrow functions and avoid using arguments?

[smorimoto]

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.

[TyOverby]

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?

[smorimoto]

@TyOverby See runtime/fs_fake.js

[smorimoto]

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.

[smorimoto]

The rest parameter and arguments show almost no difference in the benchmark results for major browsers: https://jsben.ch/BQEVR

[TyOverby]

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

[smorimoto]

The rest parameter seems to be faster for many arguments: https://jsben.ch/Krmit

[smorimoto]

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.

[TyOverby]

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

[smorimoto]

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)

[TyOverby]

Yeah, very strange.

[smorimoto]

I ran it on the latest Chrome with Apple Silicon. What's your environment?

[smorimoto]

Here is the result from Safari on the latest iOS:

[TyOverby]

I ran it on the latest Chrome with Apple Silicon. What's your environment?

Latest Chrome x86-64

[Copilot]

Copilot reviewed 5 out of 18 changed files in this pull request and generated no suggestions.

Files not reviewed (13)

• biome.json: Language not supported
• runtime/blake2.js: Evaluated as low risk
• runtime/graphics.js: Evaluated as low risk
• runtime/effect.js: Evaluated as low risk
• runtime/backtrace.js: Evaluated as low risk
• runtime/gc.js: Evaluated as low risk
• runtime/fs_node.js: Evaluated as low risk
• runtime/zstd.js: Evaluated as low risk
• runtime/md5.js: Evaluated as low risk
• runtime/weak.js: Evaluated as low risk
• runtime/sys.js: Evaluated as low risk
• runtime/marshal.js: Evaluated as low risk
• runtime/str.js: Evaluated as low risk

[Copilot]

Copilot reviewed 5 out of 17 changed files in this pull request and generated no suggestions.

Files not reviewed (12)

• biome.json: Language not supported
• runtime/js/io.js: Evaluated as low risk
• runtime/js/jslib.js: Evaluated as low risk
• runtime/js/graphics.js: Evaluated as low risk
• runtime/js/gc.js: Evaluated as low risk
• runtime/js/backtrace.js: Evaluated as low risk
• runtime/js/effect.js: Evaluated as low risk
• runtime/js/zstd.js: Evaluated as low risk
• runtime/js/fs_node.js: Evaluated as low risk
• runtime/js/blake2.js: Evaluated as low risk
• runtime/js/md5.js: Evaluated as low risk
• runtime/js/marshal.js: Evaluated as low risk

[smorimoto]

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:

[hhugo]

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.

[smorimoto]

To fix the noArguments, you need the arrow function, and to fix the useArrowFunction, you need to remove the arguments...

[hhugo]

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.

[TyOverby]

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.

[hhugo]

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]

I suspect it’s the … args syntax, but I don’t know for sure.

[hhugo]

@TyOverby, Have you run more that just micro benchmarks ? Were you able to see differences with real applications ?

[TyOverby]

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

[hhugo]

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 #1647 that I ended up reverting.

[TyOverby]

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

[hhugo]

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 #1740 and I personally find the code much more digest in its new form.

[smorimoto]

Benchmarking with CAMLBOY is probably useful: https://github.com/linoscope/CAMLBOY

[smorimoto]

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

[smorimoto]

In fact, this PR scope is included, but neither good nor bad effects seem to happen, at least for CAMLBOY. We may need a benchmark that has a negative impact.

[hhugo]

The more I look at it the more I dislike the systematic use of arrow functions. I think we should use them only when it improves readability significantly. If one show that it improves performance, we could perform the translation automatically on the fly. I believe we already have code for that.

[hhugo]

I'm going to close this one in favor of #1740. I don't really know what do to with the systematic use of arrow. Let reconsider later maybe

[smorimoto]

OK