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.

[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.

[hhugo]

I've pushed commits to only inline (small) functors in o3 with jsoo. Let's wait for the benchmarks

[hhugo]

fannkuch_redux and fft seem to take longer now. Can you take a look ? Compilation time increase everywhere but I guess we could live with that given recent improvement everywhere else

[vouillon]

For fft, it's because a function no longer gets inline because I have reduced the inlining limit from 200 down to 150.
For fankuch_redux, the function fannkuch is no longer inlined at toplevel, so it not optimized with the assumption that n = 10.

  let n = 10 in
  let _maxflips, _checksum = fannkuch n in

Inlining small functions make a significant difference for raytrace.

[hhugo]

For fft, it's because a function no longer gets inline because I have reduced the inlining limit from 200 down to 150. For fankuch_redux, the function fannkuch is no longer inlined at toplevel, so it not optimized with the assumption that n = 10.

  let n = 10 in
  let _maxflips, _checksum = fannkuch n in

Inlining small functions make a significant difference for raytrace.

Are you ok to merge in the current state ?

[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.

@TyOverby, any update on this ?

[TyOverby]

We've been trying to import these changes (well, really the base revision so that we have a good point to compare benchmarks with) and have hit a very large number of conflicts with our internal patches due to the recent PRs that have been merged. I think we're close to being ready to test this PR, my guess is next week.

[vouillon]

Are you ok to merge in the current state ?

I would prefer to wait for some feedbacks from Ty.

[rickyvetter]

We were able to run pull this in internally and performance looks very good! Substantially faster and more consistent on PRT and our other internal benchmarks. For Bonsai benchmarks we are seeing 50%-80% reduction in benchmarking times. Binary size looks like <1% increase in separate compilation and 0-2% in whole program. There are a couple outlier programs that increase in the 10-16% range.

We've reached out about a miscompilation issue on Slack and initially we believe it's unrelated to this PR directly, but it looks like applying this patch actually causes a very similar miscompilation in a program that didn't have it before. This one new case is the only test we have failing, and I suspect that if we resolve the minimal repro for the original issue, then we might also see how to resolve this new instance in this PR?

[TyOverby]

For the bonsai benchmarks, I suspect that the large improvements are due to the inlining-related memory leak being resolved by this PR

[hhugo]

We were able to run pull this in internally and performance looks very good! Substantially faster and more consistent on PRT and our other internal benchmarks. For Bonsai benchmarks we are seeing 50%-80% reduction in benchmarking times. Binary size looks like <1% increase in separate compilation and 0-2% in whole program. There are a couple outlier programs that increase in the 10-16% range.

We've reached out about a miscompilation issue on Slack and initially we believe it's unrelated to this PR directly, but it looks like applying this patch actually causes a very similar miscompilation in a program that didn't have it before. This one new case is the only test we have failing, and I suspect that if we resolve the minimal repro for the original issue, then we might also see how to resolve this new instance in this PR?

Anything to say on compilation times?

[hhugo]

Let's merge and move from there.