Update after Binaryen change to inline all single-use functions

[kripken]

Investigating the tuning of the binaryen inliner with @manoskouk , it looks like always inlining single-use functions is beneficial. In that case we can delete the function after inlining, so the code is "moved". Binaryen's default is to only inline fairly small functions in that case, but the heuristic is too conservative it seems. This PR shows what happens if we always inline such functions.

Positive findings:

• As the updated test results here show, code size shrinks by a few % on several of our reference code size tests. And the metadce tests show which function names were inlined and then removed. (If we decide to go with this, this PR would be landed with these text expectation changes after the binaryen roll.)
• The emscripten benchmark suite shows a 3.5% code size reduction. One big 30% dominates that; ignoring it using the median, the improvement is 2%, and that remains the same when looking just at the large/real-world benchmarks. Every single benchmark shrinks with this, and none regress.
• Speed improves on a few benchmarks. That is mainly on the microbenchmarks, though, so even while some of those speedups are large (one by 40%), I don't think it's very realistic. The real-world codebases in the benchmark suite do not change much in speed (but they do shrink by 2% as mentioned before).

Negative findings:

• One reason we never did this is the risk of increasing compile times on VMs, due to creating larger functions (and regalloc etc. can be nonlinear). I think that risk is much lower today since we have great baseline compilers pretty much in all wasm VMs. However, disabling the baseline compiler in v8 for example I can see some startup regressions on the benchmark suite, like 10% on base64. So there may be a downside here of slower times to reach the optimizing tier. That's just in a few of the microbenchmarks, but it is a risk.
• Even if we can delete the function after inlining we have no guarantee of a total reduction in code size. That is the second reason we never did this. It is hard to construct counterexamples manually, but the fuzzer easily does so when instrumented for that. Investigating several of them, they seem like unlikely scenarios. For example, in one case if we did not inline a function then the target of the inlining would be identical to another function and duplicate function elimination would end up helping a lot more. My impression is that these possible regressions are unlikely (especially since every single benchmark in the benchmark suite shrinks), but this is a risk.

Overall I think we should make this change. The positives are very strong, better than I expected. It would also let us deprecate and eventually remove a flag from Binaryen (unless we decide it's worth keeping?)

Thoughts?

cc @dschuff @sbc100 @aheejin @tlively @aardappel @juj @manoskouk @jakobkummerow

[sbc100]

Nice!

[aardappel]

Awesome!

I think I argued for more aggressive inlining by default in the past :)

It is an interesting question who is responsible for things like start-up times, and whether the tools side should be in the business of protecting engines against their own slowness in any current implementation. Engines may eventually have to deal with these as other toolchains produce these, and they're a moving target.

I'd definitely say that focusing on speed (which is also abstract to some extent) & size is more important.

[aheejin]

Nice! I agree it's better to do this. By the way, how can we be sure if a function is single use, in the presence of indirect calls? Do we exclude calls that can be called indirectly from multiple places?

[dschuff]

This looks nice, and I agree with what folks have said here so far. Regarding indirect calls, I think the answer is that if a function is address-taken (i.e. if it's in the table) then it can't be removed entirely, so it probably shouldn't be inlined into a direct caller (since that would be a duplication). Or maybe you could say that a direct caller is one use, and presence in the table is another use, so if we had another heuristic that said inlining/duplicating was good even for some multiple-use functions, it could still apply there.

[kripken]

Yes, the pass considers an appearance in the table as a global use (which can't be inlined into, and prevents inlining as a single-use function).

[juj]

Agreed that it would be very beneficial to have this on by default. I would recommend having it as a flag that can be disabled if explicitly desired.

[manoskouk]

I strongly support this. The current V8 policy is to allow more compile time in the optimizing compiler, and this will help us produce better code.

[kripken]

I would recommend having it as a flag that can be disabled if explicitly desired.

That can currently be done with -s BINARYEN_EXTRA_PASSES=--one-caller-inline-max-function-size=1, that is, passing a flag through to binaryen. I think that's good enough as this is not likely to be modified by many users. (But we can consider an emscripten flag perhaps if the need arises.)

[kripken]

The binaryen change landed, and this should be ready to go.

It must wait for the binaryen change to roll however.