starkware-libs / cairo

Cairo is the first Turing-complete language for creating provable programs for general computation.
Apache License 2.0
1.58k stars 488 forks source link

dev: enable Link-Time Optimization (LTO) #6471

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 a bit.

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 additionally 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 or cairo-vm config.

Basically, it can be enabled with the following lines:

[profile.release]
lto = true

For almost all known modern compilers (and other dev tools like linters, code formatters, etc.) like Rustc LTO is enabled by default for their Release builds.

Thank you.

mkaput commented 1 week ago

LTO is enabled in Scarb, so there is no incentive in enabling it here because this wouldn't bring any improvements to end users

zamazan4ik commented 1 week ago

Probably I don't understand something. I guess by "LTO enabled in Scarb" you mean this line. However, it should not influence the decision about enabling LTO for Cairo binaries itself.

... this wouldn't bring any improvements to end users

I didn't perform performance benchmarks (since I don't know how to do it yet for the project) but at least from the binary size perspective I see noticeable improvements from enabling lto = true.

Before LTO ():

drwxr-xr-x. 1 zamazan4ik zamazan4ik_new 4,4K окт  9 13:26 build
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  28M окт  9 13:27 cairo-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 6,7M окт  9 13:26 cairo-format
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  36M окт  9 13:27 cairo-language-server
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  34M окт  9 13:27 cairo-run
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  38M окт  9 13:27 cairo-test
drwxr-xr-x. 1 zamazan4ik zamazan4ik_new  81K окт  9 13:27 deps
drwxr-xr-x. 1 zamazan4ik zamazan4ik_new    0 окт  9 13:26 examples
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 1,2M окт  9 13:26 generate-syntax
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  30M окт  9 13:27 get-lowering
drwxr-xr-x. 1 zamazan4ik zamazan4ik_new    0 окт  9 13:26 incremental
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 3,0M окт  9 13:26 libcairo_lang_proc_macros.so
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 6,6M окт  9 13:26 sierra-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  30M окт  9 13:27 starknet-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 5,4M окт  9 13:26 starknet-sierra-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 1,8M окт  9 13:26 starknet-sierra-extract-code
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  15M окт  9 13:26 starknet-sierra-upgrade-validate

After LTO:

drwxr-xr-x. 1 zamazan4ik zamazan4ik_new  4,4K окт  9 13:28 build
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   15M окт  9 13:31 cairo-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  4,7M окт  9 13:29 cairo-format
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   27M окт  9 13:32 cairo-language-server
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   18M окт  9 13:32 cairo-run
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   20M окт  9 13:32 cairo-test
drwxr-xr-x. 1 zamazan4ik zamazan4ik_new   81K окт  9 13:32 deps
drwxr-xr-x. 1 zamazan4ik zamazan4ik_new     0 окт  9 13:28 examples
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new 1014K окт  9 13:29 generate-syntax
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   15M окт  9 13:31 get-lowering
drwxr-xr-x. 1 zamazan4ik zamazan4ik_new     0 окт  9 13:28 incremental
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  3,0M окт  9 13:28 libcairo_lang_proc_macros.so
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  4,8M окт  9 13:29 sierra-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   16M окт  9 13:31 starknet-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  4,0M окт  9 13:29 starknet-sierra-compile
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new  1,4M окт  9 13:29 starknet-sierra-extract-code
-rwxr-xr-x. 2 zamazan4ik zamazan4ik_new   12M окт  9 13:29 starknet-sierra-upgrade-validate

So end users will get more lightweight binaries at very least (highly-likely binaries itself will be a bit faster since LTO allows to a compiler perform better optimization decisions - but we don't have formal benchmarks for this claim yet).

mkaput commented 1 week ago

I see your perspective here.

In general, Scarb for Cairo is what Cargo is for Rust, except that the Cairo compiler is a crate statically linked into Scarb and there is no cairoc that Scarb calls. scarb is the tool that all regular users download and use. The binaries generated in this repository are mostly used internally at StarkWare and there are some users outside, but all of these are niche.

To sum up: I am not trying to block this change. I acknowledge the technical benefit (and I can confirm that there is a slight but noticeable perf improvement in Cairo compilation with LTO enabled), but will it benefit target users enough to justify the cost of build time (it's horribly slower)? I don't know.

orizi commented 1 week ago

will not do - as it is not worth it - since most users don't (and should not) use our binaries directly.