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

Improve release compile time #5113

Closed
wants to merge 2 commits into from
Closed

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jan 31, 2020

On my test, it reduced release compile time of root project from 62.9s to 54.2s.
cargo-timing files: https://send.firefox.com/download/42c022429ebe714d/#g0unPjM989ED38syPZU3Fg
changelog: none

@flip1995
Copy link
Member

I don't think we want opt-level = 0 on Clippy, since then the packaged Clippy would also be compiled with opt-level = 0, wouldn't it?

@flip1995
Copy link
Member

And I think, the only time Clippy gets compiled in release mode is when shipping it.

@flip1995
Copy link
Member

Or am I missing something?

@tesuji
Copy link
Contributor Author

tesuji commented Jan 31, 2020

packaged Clippy would also be compiled with opt-level = 0

No, it still be compile with optimization. Only build scripts (build.rs), proc-macro and theirs deps
are compile with no optimization. https://doc.rust-lang.org/nightly/cargo/reference/profiles.html#overrides

only time Clippy gets compiled in release mode

It's a fair point. But I think a case when someone build clippy from source.

@flip1995
Copy link
Member

flip1995 commented Feb 1, 2020

It's a fair point. But I think a case when someone build clippy from source.

clippy_dummy cannot be compiled.

clippy_dev had worse release compile times on my system with this change.

clippy itself has this timing from a clean build:

# With this change
cargo build --release  451.07s user 7.01s system 239% cpu 3:11.53 total
# Without this change
cargo build --release  515.73s user 5.77s system 253% cpu 3:25.63 total

Which is only ~7% faster. I don't think that this matters for people that want to compile Clippy from source.

@tesuji
Copy link
Contributor Author

tesuji commented Feb 1, 2020

clippy_dev had worse release compile times on my system with this change.

You're right. I just measure it now. In my machine, it doesn't make a noticeable different in compilation time. Remove it then.

Which is only ~7% faster. I don't think that this matters for people that want to compile Clippy from source.

At least it is an improvement. If you think this is a risky or unknown trade-off, I would close this PR.

@flip1995
Copy link
Member

flip1995 commented Feb 1, 2020

At least it is an improvement. If you think this is a risky or unknown trade-off, I would close this PR.

I really don't know, how big the difference is. We could try to run it on some crates and time it with and without this change.

@flip1995 flip1995 added S-needs-discussion Status: Needs further discussion before merging or work can be started S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 1, 2020
@phansch
Copy link
Member

phansch commented Mar 5, 2020

To move the discussion a bit forward:

Would it help if we had a performance dashboard where we run Clippy on a few crates or code examples? Maybe integration in perf.rust-lang.org is something that's possible?

@flip1995
Copy link
Member

flip1995 commented Mar 5, 2020

I think that would help to optimize Clippy.

@bors
Copy link
Contributor

bors commented Mar 14, 2020

☔ The latest upstream changes (presumably #5314) made this pull request unmergeable. Please resolve the merge conflicts.

@mati865
Copy link
Contributor

mati865 commented Mar 14, 2020

Do we know if change like this PR won't make Clippy run slower on large code bases?

@tesuji
Copy link
Contributor Author

tesuji commented Mar 14, 2020

This PR is about compilation time. IMO it shouldn't affect runtime performance.
If it does, maybe that a question for cargo team and its documentation because
I see nothing mention that this setting affects codegen.

@Manishearth
Copy link
Member

Yeah not convinced that this is very useful to have, and given that it's making clippy_dev worse perhaps we should leave things as is and close this? In general it is worth playing around with build_override though, thanks for investigating!

@tesuji
Copy link
Contributor Author

tesuji commented Mar 30, 2020

Ok fine

@tesuji tesuji closed this Mar 30, 2020
@tesuji tesuji deleted the release-comptime branch May 24, 2024 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants