search

Shnatsel / wondermagick

Memory-safe replacement for imagemagick
29 stars 0 forks source link

Enable Link-Time Optimization (LTO) #5

Closed zamazan4ik closed 1 week ago

zamazan4ik commented 1 week ago

Hi!

I noticed that in the Cargo.toml file Link-Time Optimization (LTO) for the project is not enabled. I suggest switching it on since it will reduce the binary size (always a good thing to have) and will likely improve the application's performance.

I suggest enabling LTO only for the Release builds so as not to sacrifice the developers' experience while working on the project since LTO consumes an additional amount of time to finish the compilation routine. If you think that a regular Release build should not be affected by such a change as well, then I suggest adding an additional dist or release-lto profile where in addition to regular release optimizations LTO will also be added. Such a change simplifies life for maintainers and others interested in the project persons who want to build the most performant version of the application. Using ThinLTO should also help to reduce the build-time overhead with LTO. If we enable it on the Cargo profile level, users, who install the application with cargo install, will get the LTO-optimized version "automatically". E.g., check cargo-outdated Release profile.

Basically, it can be enabled with the following lines:

[profile.release]
lto = true

I have made quick tests (Fedora 41, Rustc 1.82) by adding lto = true to the Release profile. The binary size reduction is from 6.1 Mib to 5.0 Mib. I believe that performance also should be improved but I didn't perform measurements.

Thank you.

Shnatsel commented 1 week ago

Right now this isn't done for several reasons:

  1. We're still in a prototype stage, and an apples-to-apples comparison against imagemagick is important. For a fair comparison I would have to compile imagemagick with LTO too, and I cannot be bothered to do that. That's also not how most imagemagick binaries in the wild are compiled.
  2. Low binary size is an explicit non-goal. Storage is cheaper than ever. In fact, I'm shocked it's just 6MB right now, with all these different formats enabled already.
  3. High performance is a goal, but the effects of LTO on performance are quite unpredictable. I've seen fat LTO regress performance about as often as it improved it.
zamazan4ik commented 1 week ago

We're still in a prototype stage, and an apples-to-apples comparison against imagemagick is important. For a fair comparison I would have to compile imagemagick with LTO too, and I cannot be bothered to do that.

In this case, I suggest creating a dedicated profile like [profile.compare-with-im] where you can maintain the closest to the IM compiler options. For more optimized builds you can use more optimizations than IM does. As an example of multiple profiles I have this: https://github.com/girlbossceo/conduwuit/blob/main/Cargo.toml#L547

Low binary size is an explicit non-goal. Storage is cheaper than ever. In fact, I'm shocked it's just 6MB right now, with all these different formats enabled already.

Sure! But if we can do it without hurting performance (see the benchmarks below) - it would be nice to have it.

High performance is a goal, but the effects of LTO on performance are quite unpredictable. I've seen fat LTO regress performance about as often as it improved it.

I performed the tests according to the officially available benchmarks for the app: no performance regressions (nor improvements) were detected for wondermagick: https://gist.github.com/zamazan4ik/e6715b0aa352e9c16df68c6ba59c2eae

However, if you still don't think that LTO is valuable for the project - feel free to close the issue.

Shnatsel commented 1 week ago

Thanks for measuring the impact!

Since it doesn't affect performance I don't think enabling it by default would be valuable, at least at this stage. We can revisit this if/when it does have a measurable effect on benchmarks.