Rewrite inlining pass

[vouillon]

No description provided.

[hhugo]

I've pushed a fixup to the testsuite.
We should check how this PR affects functor heavy programs (something using core maybe).
@TyOverby could you tests this PR on your side ?

[hhugo]

We need a changelog entry

[hhugo]

I'm not certain I read the benchmark correctly.
It seems that partial render table sees a code size increase of 10%, memory increase of ~50%, compilation time increase of 30% for not runtime improvement.

[hhugo]

Maybe we can wait for #1962 to get better measurements.

[hhugo]

We don't have the latest benchmark.
The last result we have shows a runtime regression for ocamlc (maybe some noise ?).

With this PR, we seem to double the time spent in inline. We can probably live with that.

[hhugo]

@vouillon, we need to investigate the macos failure.
Can we do anything to reduce compilation time with --opt 1 ?

@TyOverby, gentle ping on testing this PR.

This PR definitely need a changelog entry

[hhugo]

@vouillon, we need to investigate the macos failure.

It could be an issue with node 24.

[vouillon]

@vouillon, we need to investigate the macos failure.

The test pass a lazy list to a function. This value is unfolded into something pretty large. The function is no longer inlined (we avoid inlining functions containing loops at toplevel since we expect that the toplevel code will not get optimized). Since node retains function parameters, the value is not garbage-collected and node eventually runs out of memory.

[vouillon]

I need to go through this code again and add some comments.
I'll see whether I can reduce compilation time with --opt 1.

[hhugo]

@vouillon, we need to investigate the macos failure.

The test pass a lazy list to a function. This value is unfolded into something pretty large. The function is no longer inlined (we avoid inlining functions containing loops at toplevel since we expect that the toplevel code will not get optimized). Since node retains function parameters, the value is not garbage-collected and node eventually runs out of memory.

Downgrading to node 23 fixes the CI

[hhugo]

There is a large increase for maxresident bewteen node 23 and node 24
node 23

$ /bin/time node _build/default/compiler/tests-ocaml/match-exception/streams.bc.js
iter_stream with handler case (match) is tail recursive
3.79user 0.52system 0:04.00elapsed 107%CPU (0avgtext+0avgdata 2077648maxresident)k
0inputs+0outputs (0major+510186minor)pagefaults 0swaps

node 24

$ /bin/time node _build/default/compiler/tests-ocaml/match-exception/streams.bc.js
iter_stream with handler case (match) is tail recursive
3.77user 0.92system 0:02.89elapsed 162%CPU (0avgtext+0avgdata 3259072maxresident)k
0inputs+0outputs (0major+825840minor)pagefaults 0swaps

[TyOverby]

Apologies for the delay; I didn't see this thread for a while. We should have some test and benchmark results ready for you next week.

[TyOverby]

out of curiosity, what was the osx / node-24 issue? consuming too much memory?

[hhugo]

Apologies for the delay; I didn't see this thread for a while. We should have some test and benchmark results ready for you next week.

Many improvements landed on master in the past few days. It would be nice to test both base and tip of the PR so we can understand the impact of the PR alone

[vouillon]

out of curiosity, what was the osx / node-24 issue? consuming too much memory?

@TyOverby See my comment above.

[vouillon]

I'm not certain I read the benchmark correctly. It seems that partial render table sees a code size increase of 10%, memory increase of ~50%, compilation time increase of 30% for not runtime improvement.

Right, the aggressive inlining of functors does not really seem to result into any runtime improvement with js_of_ocaml. So it is not enabled only with wasm_of_ocaml.