nroi / flexo

a central pacman cache
MIT License
172 stars 10 forks source link

Optimize release builds #104

Closed obj-obj closed 1 year ago

obj-obj commented 1 year ago

Before/after: image

nroi commented 1 year ago

I'm very hesitant about changing low level configurations like compiler options. The defaults are often chosen with a good reason, so I change such values only if they solve an actual issue, or if it's commonly agreed that the default values are just not optimal.

After a quick research, I'm not convinced this is the case with the options in your PR. strip=true means stacktraces are much less useful when you run the program with RUST_BACKTRACE=full, codegen-units means compile times will be slower, even though the performance gains are not guaranteed. Not sure about opt-level and lto, but before I'm convinced that this has a noticeable impact, I'll stick with the defaults.

obj-obj commented 1 year ago

I'm very hesitant about changing low level configurations like compiler options. The defaults are often chosen with a good reason, so I change such values only if they solve an actual issue, or if it's commonly agreed that the default values are just not optimal.

After a quick research, I'm not convinced this is the case with the options in your PR. strip=true means stacktraces are much less useful when you run the program with RUST_BACKTRACE=full, codegen-units means compile times will be slower, even though the performance gains are not guaranteed. Not sure about opt-level and lto, but before I'm convinced that this has a noticeable impact, I'll stick with the defaults.

This isn't really meant to improve performance (although it likely does a little bit), it's mainly to reduce binary size

And wouldn't you be using a debug build when testing (so strip on the release build isn't a problem)?

I've gotten all of these flags from https://github.com/johnthagen/min-sized-rust, and this stuff isn't the default because the Rust devs want compile time to be better by default for some reason (these aren't unstable options or anything)

Also opt-level already defaults to 3, idk why I put that in there

A lot of software in rust uses these flags or something similar, e.g. rnote, helix, gitui, lazymc, exa, etc.

obj-obj commented 1 year ago

@nroi pinging you in case my previous comment didnt send a notification or something (idk if it still sends notifications after the PR is closed)

nroi commented 1 year ago

This isn't really meant to improve performance (although it likely does a little bit), it's mainly to reduce binary size

Storage is cheap these days, so I don't see a big advantage of that. I understand that Arch users tend to strive for minimalism, so if it were possible to make the binary smaller without any drawbacks, that would be nice. But there are tradeoffs involved.

And wouldn't you be using a debug build when testing (so strip on the release build isn't a problem)?

Not necessarily. If you take a look at this file for example, this is used during testing, but I'm using the --release flag there. Those are integration tests, and I generally want integration tests to resemble the reality as close as possible, and the reality is that users compile flexo with --release.

and this stuff isn't the default because the Rust devs want compile time to be better by default for some reason

Yes, and I also want better compile times, because I recompile flexo frequently for the integration tests. Also, many users use the AUR package where they compile flexo, so they will also notice compile times.

A lot of software in rust uses these flags

I've also seen popular projects that don't use those flags, like ripgrep.

I appreciate it that people come up with suggestions to improve Flexo, and I completely understand why other maintainers choose to use those options. But binary size is just not a priority for me at the moment. I want to improve the maintainability and developer-friendliness of Flexo, and that requires decent compile times and meaningful stacktraces.

obj-obj commented 1 year ago

This isn't really meant to improve performance (although it likely does a little bit), it's mainly to reduce binary size

Storage is cheap these days, so I don't see a big advantage of that. I understand that Arch users tend to strive for minimalism, so if it were possible to make the binary smaller without any drawbacks, that would be nice. But there are tradeoffs involved.

And wouldn't you be using a debug build when testing (so strip on the release build isn't a problem)?

Not necessarily. If you take a look at this file for example, this is used during testing, but I'm using the --release flag there. Those are integration tests, and I generally want integration tests to resemble the reality as close as possible, and the reality is that users compile flexo with --release.

and this stuff isn't the default because the Rust devs want compile time to be better by default for some reason

Yes, and I also want better compile times, because I recompile flexo frequently for the integration tests. Also, many users use the AUR package where they compile flexo, so they will also notice compile times.

A lot of software in rust uses these flags

I've also seen popular projects that don't use those flags, like ripgrep.

I appreciate it that people come up with suggestions to improve Flexo, and I completely understand why other maintainers choose to use those options. But binary size is just not a priority for me at the moment. I want to improve the maintainability and developer-friendliness of Flexo, and that requires decent compile times and meaningful stacktraces.

imo the release profile should only be for actual release builds, if you need some optimization but not all on the devbuilds you can just make another profile called devbuild or something, or do what helix does and make a separate profile called opt for release builds (and put cargo build --profile opt in the AUR package or something)

We can just agree to disagree ig