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.

Sign up for GitHub

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

Jump to bottom

Tidy is incredibly slow #127453

Closed
Noratrieb opened this issue Jul 7, 2024 · 6 comments
Closed

Tidy is incredibly slow #127453

Noratrieb opened this issue Jul 7, 2024 · 6 comments
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@Noratrieb
Copy link
Member

Noratrieb commented Jul 7, 2024 •
edited
Loading

Running git push with the tidy check hook just hangs. perf top shows that it's doing lots of string operations:

 34.53%  rust-tidy         [.] alloc::str::<impl str>::to_uppercase
  12.34%  rust-tidy         [.] core::unicode::unicode_data::conversions::to_upper
  10.02%  rust-tidy         [.] alloc::string::String::push
   8.63%  libc.so.6         [.] __memcmp_avx2_movbe

x test tidy takes 97s
???
@Noratrieb Noratrieb added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Jul 7, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 7, 2024
@Noratrieb
Copy link
Member Author

Noratrieb commented Jul 7, 2024 •
edited
Loading

#127428 is at fault, a revert makes the perf good again. that cute to_uppercase and also linear search is NOT very efficient :D
@donno2048 @albertlarsan68 (don't worry, I'll put up a fix)

@Noratrieb
Copy link
Member Author

yeah, that array contains 2084 different elements. moving the to_uppercase out of the loop still takes 24s. i'm just gonna revert this commit, the regexset was made for this.

@Noratrieb Noratrieb mentioned this issue Jul 7, 2024
Merged
@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 7, 2024
@donno2048
Copy link
Contributor

I still think we should not revert the commit as it fixes issues with case alteration.

We should simply do the check on trimmed only if the regex test yields True.

@Noratrieb
Copy link
Member Author

Noratrieb commented Jul 7, 2024 •
edited
Loading

sounds fine, feel free to do a follow-up doing that. I just wanted to get something working again quickly

@jieyouxu
Copy link
Member

jieyouxu commented Jul 7, 2024

I still think we should not revert the commit as it fixes issues with case alteration.

It's perfectly fine to revert then reland the fixes!

@donno2048 donno2048 mentioned this issue Jul 7, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 7, 2024
@bors
Auto merge of rust-lang#127455 - Nilstrieb:blazing-tidy, r=jieyouxu
959a2eb 
Make tidy problematic const checking fast again

fixes pathological tidy performance described in rust-lang#127453 by reverting rust-lang#127428

i think anyone can approve this ASAP, it makes working on this repo significantly worse.
@Noratrieb
Copy link
Member Author

has been fixed. the new PR to make it better is open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jieyouxu @rustbot @Noratrieb @donno2048