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

Build quine-mc_cluskey with opt-level=3 in dev builds #13408

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Sep 17, 2024

While doing some profiling I noticed that debug clippy running on the clippy_lints crate spends 35s out of 160s in one specific code path of nonminimal_bool, which seemed a bit excessive.

I've found that just enabling optimizations for quine-mc_cluskey (used by nonminimal_bool) cuts down the part that took 35s to 3s

While this doesn't really change anything for users, this helps dogfood a bit as it cuts off about half a minute of runtime (in some of my tests, at least).

Something similar was attempted in #10576, however that involved compiling everything in release mode including clippy itself, whereas this only affects a single dependency that's compiled in parallel with something that takes longer so this should hopefully not have a negative impact in any case (and changing clippy doesn't require recompiling that dependency)

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2024

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 17, 2024
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, good catch. This makes sense. I can't test this locally right now, so I leave the final approval to @dswij

@y21
Copy link
Member Author

y21 commented Sep 18, 2024

(I just added a comment for context since it might not be obvious why this is done)

Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Tested locally on my machine and it seems to hit that sweet 30+s mark cut 😄

@dswij
Copy link
Member

dswij commented Sep 23, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2024

📌 Commit 431f7d6 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 23, 2024

⌛ Testing commit 431f7d6 with merge 59bac6a...

@bors
Copy link
Contributor

bors commented Sep 23, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 59bac6a to master...

@bors bors merged commit 59bac6a into rust-lang:master Sep 23, 2024
5 checks passed
@flip1995
Copy link
Member

@y21 I had to revert this during the sync rust-lang/rust#130778 in commit rust-lang/rust@f38b569 as this caused warnings when building in the Rust repo:

warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /home/pkrones/rust-lang/rust/src/tools/clippy/Cargo.toml
workspace: /home/pkrones/rust-lang/rust/Cargo.toml

Those would be annoying to the Rust devs. I didn't know how best to fix this otherwise and don't want to delay the sync further.

@y21
Copy link
Member Author

y21 commented Sep 24, 2024

Ah, that's unfortunate... I'll try to get this to work without affecting rust-lang/rust, but yeah, reverting it just to unblock the sync for now sounds fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants