Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Isn't lto = true a bit of an overkill? #131

Open
dpc opened this issue Nov 21, 2018 · 9 comments
Open

Isn't lto = true a bit of an overkill? #131

dpc opened this issue Nov 21, 2018 · 9 comments

Comments

@dpc
Copy link
Contributor

dpc commented Nov 21, 2018

It increases incremental compilation time from 0:17.27 to 1:00.80 on my machine and it doesn't seem to me that kak-lsp routing network calls back and forth is going to benefit from any marginal runtime speedup.

On the other hand faster recompilation time would mean users of plug.kak will wait shorter on :plug-update.

@ul
Copy link
Collaborator

ul commented Nov 21, 2018

It's mostly about the size, not performance, because consuming kak-lsp as a pre-compiled binary is still the primary use-case. And though it's only 12% / 0.8MB difference, cutting it costs me nothing when I prepare release, I'd like to continue to do it.

I wonder if cargo allows to make profile configuration dependent on feature argument. Then plug.kak usage could look like plug "ul/kak-lsp" do %{cargo build --release --features "no-lto"} and benefit from faster build.

@dpc
Copy link
Contributor Author

dpc commented Nov 21, 2018

I'm reasonably sure you can do something with ./.cargo/config file that would at least let you compile locally the packaged release with LTO, and not turn it on by default for people that compile from source.

@maximbaz
Copy link
Contributor

As a package maintainer I also prefer to build with LTO, I don't mind an extra build time and I'm building in an isolated environment so manually creating some config files would be less than ideal.

Is it possible to enable or disable LTO via a command line argument to cargo build? I tried RUSTFLAGS="-C lto" cargo build --release --locked but this fails the build.

@dpc
Copy link
Contributor Author

dpc commented Nov 22, 2018

rust-lang/cargo#4349 :/

@dpc
Copy link
Contributor Author

dpc commented Nov 22, 2018

@maximbaz
Copy link
Contributor

Uhh 😞
I'll leave it up to @ul to decide whether to keep it as it is for the time being or not, from my point of view it's more beneficial to keep LTO enabled at this point.

By the way, does plug.kak follows official releases of kak-lsp or forces you to stay on master end rebuild on every commit?

@ul
Copy link
Collaborator

ul commented Nov 22, 2018

I lean towards keeping LTO enabled by default for now as releases are considered to be primary way to use kak-lsp. That said, I'm totally open for having any extra configs, scripts, whatever which would make plug.kak user life easier. E.g. instead of do %{cargo build --release} they would be able to use smth like this: do %{ ./build-binary-quickly-and-generate-plug-kak-compatible-kakoune-script.sh }

@maximbaz
Copy link
Contributor

maximbaz commented Nov 22, 2018

This could be a Makefile, ideally you and me would just run make to build LTO-enabled binary, while plug users would run do %{ make lto=no }

If you find a good way to enable/disable LTO on the spot, I can help with the Makefile, not an expert myself but I know where to find examples 😄

@daboross
Copy link
Contributor

Hopefully this can be fixed by a custom release profile once rust-lang/cargo#6988 is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants