Change bevy_math to use inline instead of inline(always)

[greeble-dev]

Objective

Move bevy_math closer to recommended inlining practices, and avoid problems with debuggers and optimising for size.

Background

Some bevy_math modules apply #[inline(always)] to almost every function. This has downsides for some users - it can prevent optimising for size, and can stop the debugger from stepping into functions.

I can't find any source that advises inline(always) by default - the most common advice is "use rarely and only after profiling" (example: std lib guide). I've poked around the bevy_math history but couldn't find anything that explains why inline(always) was chosen.

Solution

This PR changes all instances of #[inline(always)] to #[inline].

The change is very unlikely to make any difference in optimised builds - almost all the functions are tiny so they're going to be inlined either way. Benchmarks showed no difference.

The change can sometimes decrease performance in opt-level = 0 builds - one math heavy microbenchmark took a -10% hit. But this is arguably the right trade-off if it lets the user step into functions in the debugger.

Overall, I think this is the safer default for most users. inline(always) has several concrete downsides, while inline has some trade-offs but no clear downsides.

The change also adds a new bevy_math benchmark with a mix of small and large functions. Not sure if this is justified.

Alternatives

The change could have been taken further.

• Remove #[inline] from heftier functions like BoundingSphere::from_point_cloud.
  • Could help optimising for size. I left this out to keep things simple.
• Remove #[inline] entirely.
  • I think this is likely to be a good thing, but it needs more testing and would probably be controversial.

Testing

cargo bench -p benches --bench math

Also:

• Checked benchmark disassembly to confirm what was inlined.
• Compiled alien_cake_addict with various optimisation levels to check there weren't major differences in size.

More details in comment 1, comment 2.

[mockersf]

I've poked around the bevy_math history and couldn't find any discussion or profiling.

That's older than that: https://discord.com/channels/691052431525675048/692572690833473578/796109019398930453

Cart guidelines were to #[inline(always)] every small function by default.

[greeble-dev]

That's older than that: https://discord.com/channels/691052431525675048/692572690833473578/796109019398930453

Cart guidelines were to #[inline(always)] every small function by default.

I'm not sure if that discussion is saying "always use inline(always)" or "always use inline"? Might be worth noting that there's been some Rust changes since then - small functions are now inlined across crates automatically: rust-lang/rust#116505.

For what it's worth, I should have phrased my comment differently. I didn't mean to imply that there had been no discussion - just that I failed to find it.

[tbillington]

It would be helpful to show a benchmark run before and after, since this PR is changing code specifically related to performance, have you checked the git blames on those inline attributes to make sure there wasn't a good reason?

---

Another way to think of #[inline(always)] for small/helper functions like this is it's almost a more ergonomic replacement for C style macros/macro_rules for code that should be inline, but we don't want to spam macros everywhere for.

For example, It's hard to think of a scenario where you want something as simple as field dereferencing to be behind a function call, and this PR includes changes to remove inline(always) from some of these.

#[inline(always)]
fn center(&self) -> Self::Translation {
    self.center
}

---

Broadly I think your reasoning isn't unsound, and there's likely quite a few places the inline(always) could be relaxed to just inline or none at all.

However this PR is changing hundreds of functions without showing due diligence, there is a lot of assumptions being made here with no numbers to show. It would be appreciated to show benchmark run before/after, at least for math heavy stuff. And potentially compiled binary sizes?

[greeble-dev]

I mentioned benchmarks and binary sizes only briefly as the TLDR was "no difference in optimised builds". Maybe I should have expanded a bit:

• I added a new benchmark that tests the largest functions I could find (Aabb/BoundingSphere::from_point_cloud) and a few smaller ones.
  • With the default profile and opt-level = "s" there was no difference to bevy_math benchmarks.
    • I also checked the assembly of the new benchmark and there were only minor differences - no extra function calls.
  • With opt-level = 0, the new benchmark was 10% slower.
    • I'd suggest that's a reasonable trade-off for making the function easier to debug, and making optimise-for-size more useful.
• I built alien_cake_addict with opt-level 0, 1, 3 and s.
  • Binary sizes were identical except for a few KB reduction with opt-level = 0.
• Tested on x64, Zen 4, default cpu target.

This is far from conclusive - they're microbenchmarks and alien_cake_addict isn't math heavy - but when the largest functions are still inlineable that suggests to me the risks are low.

On bevy_math history, I went through a bunch of blames and PRs and couldn't find anything that explained why it's using inline(always). Not an exhaustive check though.

I've done a few more checks since filing the PR:

• Confirmed that inline(always) prevents LLDB stepping into Aabb3d::from_point_cloud with opt-level = 0. inline is fine.
  • Not a huge deal, but being able to debug a function like this means the PR has some upside.
• Reviewed some crates that I'd expect to have gone through lots of performance testing.
  • In glam and std the ratio of inline(always) to inline is very low - 1% and 2% respectively.
  • hashbrown is 10%, mostly on non-trivial functions, some with comments about requiring inlining to eliminate dynamic dispatch.

That's suggestive but still not conclusive. So I think there's another way to frame my argument - what if the situation was reversed? What if bevy_math was currently using inline and this PR changed it to inline(always)? That would go against common advice, add risk around debugging and optimise-for-size, and I wouldn't be able to show any benefits in benchmarks. Would that PR be accepted?

For this PR, there's one case I'm ambivalent about: the libm wrapper calls in ops.rs. I don't think there's any benefit to them being inline(always), but there isn't much risk either, and I haven't tested them. I can change them back if preferred.

[greeble-dev]

Did some more testing around optimise for size in case it's useful:

• Tested on x64, default CPU target.
• The test was to call BoundingSphere::from_point_cloud, since that's fairly chonky for a math function (90 instructions when not inlined).
  • The function was called ten times in separate functions to try and avoid the compiler favouring inlining due to low call count.
• Tried opt-level 3 and s, with and without an #[inline] annotation.
• Results:
  • opt-level = 3, #[inline]: inlined.
  • opt-level = 3, no annotation: inlined.
  • opt-level = s, #[inline]: inlined.
  • opt-level = s no annotation: not inlined.
    • The internals of from_point_cloud were still inlined.
    • Also tried changing the test to only call from_point_cloud once - result was inlined.
• Conclusions:
  • Supports the view that #[inline(always)] is not required, even for chonky functions.
  • Larger functions should probably avoid #[inline] to support optimising for size.
    • But it's debatable and I'm not going to try it in this PR.

[tbillington]

Thanks for providing more info, appreciated!

That's suggestive but still not conclusive. So I think there's another way to frame my argument - what if the situation was reversed? What if bevy_math was currently using inline and this PR changed it to inline(always)?

It's a good point.