Skip to content

Override rustc version in ui and mir-opt tests to get stable hashes #92363

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
Merged
merged 2 commits into from
Jan 21, 2022
Merged

Override rustc version in ui and mir-opt tests to get stable hashes #92363

merged 2 commits into from
Jan 21, 2022

Conversation

@the8472
Copy link
Member

@the8472 the8472 commented Dec 28, 2021

Building a dozen separate regexps for each test in compiletest consumes significant amounts of CPU cycles.

UI test timings on my machine:

OLD: 39.63s
NEW: 30.27s

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 28, 2021
@rust-highfive
Copy link
Contributor

rust-highfive commented Dec 28, 2021

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the less-compiletest-normalization branch from 664a79e to e0611f7 Compare December 28, 2021 18:26
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the less-compiletest-normalization branch from e0611f7 to d057342 Compare December 28, 2021 19:25
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the less-compiletest-normalization branch from d057342 to 9d4ee23 Compare December 28, 2021 20:15
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the less-compiletest-normalization branch from 9d4ee23 to e49b06d Compare December 28, 2021 21:05
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the less-compiletest-normalization branch from e49b06d to 038a82d Compare December 28, 2021 22:12
tmiasko
tmiasko reviewed Dec 29, 2021
View reviewed changes
@the8472 the8472 force-pushed the less-compiletest-normalization branch from 038a82d to 2a818de Compare December 29, 2021 21:47
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the less-compiletest-normalization branch from 2a818de to 4694f87 Compare December 29, 2021 22:09
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 5, 2022

It looks like this is mixing the incr-comp hash noise removal with a bunch of unrelated(?) changes to move regex's to lazy_static! -- is that right? Can we at least mention that in the PR description + commit message, or ideally split that bit out into a separate commit? (Happy to review it as part of this PR, if you want).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2022
@the8472
Copy link
Member Author

the8472 commented Jan 5, 2022 •
edited
Loading

It looks like this is mixing the incr-comp hash noise removal with a bunch of unrelated(?) changes to move regex's to lazy_static! -- is that right?

My goal was to improve performance. The bottleneck was regex creation. This required
a) making fixed regular expressions lazy_static
b) removing value-dependent regular expressions generated for each test, i.e. do less normalizing
c) which in turn required RUSTC_FORCE_INCR_COMP_ARTIFACT_HEADER to stabilize them so we can live without normalization

But sure, I can extract a) into another commit.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 5, 2022

Yeah, that makes sense (and is a great goal, thanks!) -- just want to make sure I'm reviewing with the right things in mind. Splitting out (b) from (c), presuming it is sort of independent which it sounds like it would be for at least some of the normalizing variables may also make sense.

@the8472
Copy link
Member Author

the8472 commented Jan 5, 2022

presuming it is sort of independent which it sounds like it would be for at least some of the normalizing variables may also make sense.

That'd be more work to tease them apart since I just relied on bless and the test results to see which files need to get updated and didn't pay attention to which change affected what. I'd like to avoid that if it's ok.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 5, 2022

Sounds good, no worries.

@the8472 the8472 force-pushed the less-compiletest-normalization branch from 4694f87 to e4e3b64 Compare January 5, 2022 05:41
@the8472 the8472 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 7, 2022
Mark-Simulacrum
Mark-Simulacrum approved these changes Jan 10, 2022
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me with a comment added

@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2022
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2022
bjorn3
bjorn3 reviewed Jan 18, 2022
View reviewed changes
@bjorn3
Copy link
Member

bjorn3 commented Jan 18, 2022

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 18, 2022
@bors
Copy link
Collaborator

bors commented Jan 18, 2022

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

@the8472 the8472 force-pushed the less-compiletest-normalization branch from f081c21 to 852085d Compare January 18, 2022 21:05
@the8472
Copy link
Member Author

the8472 commented Jan 18, 2022

rebased, updated the variable and re-blessed the test outputs.

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jan 18, 2022

📌 Commit 852085dd257afb14b823113101733cab26c58fc9 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2022
@ehuss
Copy link
Contributor

ehuss commented Jan 20, 2022

@bors r-

The tests need to be reblessed due to #92316

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 20, 2022
@the8472
Override rustc version in ui and mir-opt tests to get stable hashes
8985e5e 
Building a dozen separate regexps for each test in compiletest consumes significant amounts of CPU cycles.
Using `RUSTC_FORCE_INCR_COMP_ARTIFACT_HEADER` stabilizes hashes calcuated for the individual tests so
no test-dependent normalization is needed. Hashes for the standard library still change so some
normalizations are still needed.
@the8472
use lazy_static to compile Regex::new instances once instead on eac…
7a5796d 
…h test
@the8472 the8472 force-pushed the less-compiletest-normalization branch from 852085d to 7a5796d Compare January 20, 2022 23:28
@the8472
Copy link
Member Author

the8472 commented Jan 20, 2022

rebased and reblessed

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jan 20, 2022

📌 Commit 7a5796d has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2022
@pierwill
Copy link
Contributor

pierwill commented Jan 21, 2022

Thanks for this, @the8472! Glad to see we can scale back some of the more heavy-handed normalization needed to get #89836 merged. :)

@matthiaskrgr
Copy link
Member

matthiaskrgr commented Jan 21, 2022

@bors rollup=never

@bors
Copy link
Collaborator

bors commented Jan 21, 2022

⌛ Testing commit 7a5796d with merge 17d29dc...

@bors
Copy link
Collaborator

bors commented Jan 21, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 17d29dc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2022
@bors bors merged commit 17d29dc into rust-lang:master Jan 21, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 21, 2022
@rust-timer
Copy link
Collaborator

rust-timer commented Jan 22, 2022

Finished benchmarking commit (17d29dc): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@tmiasko tmiasko tmiasko left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.60.0

Development

Successfully merging this pull request may close these issues.