Skip to content

Combine two loops in check_match #80367

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 1 commit into from
Jul 27, 2021
Merged

Combine two loops in check_match #80367

merged 1 commit into from
Jul 27, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 25, 2020 •
edited
Loading

Suggested by Nadrieril in
#79051 (comment).

Opening to get a perf run. Hopefully this code doesn't require everything in the
first loop to be done before running the second! (It shouldn't though.)

cc @Nadrieril

@camelid camelid added A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns I-compiletime Issue: Problems and improvements with respect to compile times. labels Dec 25, 2020
@rust-highfive
Copy link
Contributor

r? @varkor

(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 25, 2020
@camelid camelid mentioned this pull request Dec 25, 2020
Merged
3 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@varkor varkor 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 Dec 26, 2020
@camelid
Copy link
Member Author

camelid commented Feb 14, 2021

Is there a way to combine these two loops after all given the borrowck error?

@camelid camelid 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 Feb 16, 2021
@varkor
Copy link
Contributor

varkor commented Feb 17, 2021

r? @Nadrieril

@rust-highfive rust-highfive assigned Nadrieril and unassigned varkor Feb 17, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 16, 2021

@camelid I think you can fix the error by removing pattern_arena from MatchCheckCtxt and using the field on MatchVisitor instead.

@jyn514 jyn514 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 Apr 16, 2021
@camelid camelid force-pushed the check_match-combine-loop branch from 5c65386 to b68d851 Compare July 23, 2021 03:10
@camelid
Copy link
Member Author

camelid commented Jul 23, 2021

Starting with a rebase.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member Author

camelid commented Jul 23, 2021

@camelid I think you can fix the error by removing pattern_arena from MatchCheckCtxt and using the field on MatchVisitor instead.

Ok, I tried that, but I ran into a lot of errors because MatchCheckCtxt.pattern_arena is used in a lot of places. But, I just noticed that the function that was causing the errors because it took &mut self didn't actually need a mutable reference! Removing the mut fixed the borrow-check errors. So it ended up being a much smaller change than I thought ;)

@camelid camelid force-pushed the check_match-combine-loop branch from b68d851 to c3a03ae Compare July 23, 2021 03:51
@camelid camelid 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 Jul 23, 2021
@camelid camelid marked this pull request as ready for review July 23, 2021 03:52
@camelid
Copy link
Member Author

camelid commented Jul 23, 2021

I'll run perf on this after CI passes.

@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 23, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 23, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 23, 2021
@camelid
Copy link
Member Author

camelid commented Jul 23, 2021

bors seems to have lost track of this PR. Closing and reopening.

@camelid camelid closed this Jul 23, 2021
@camelid camelid reopened this Jul 23, 2021
@camelid
Copy link
Member Author

camelid commented Jul 23, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Collaborator

bors commented Jul 23, 2021

⌛ Trying commit c3a03ae with merge cc556f91642435dd5ba31f27af2d573b5f9e1526...

@bors
Copy link
Collaborator

bors commented Jul 23, 2021

☀️ Try build successful - checks-actions
Build commit: cc556f91642435dd5ba31f27af2d573b5f9e1526 (cc556f91642435dd5ba31f27af2d573b5f9e1526)

@rust-timer
Copy link
Collaborator

Queued cc556f91642435dd5ba31f27af2d573b5f9e1526 with parent 0443424, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (cc556f91642435dd5ba31f27af2d573b5f9e1526): comparison url.

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

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 23, 2021
@jyn514 jyn514 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 23, 2021
@Nadrieril
Copy link
Member

So it ended up being a much smaller change than I thought ;)

Ah yeah that's the change I hoped you could get, I was surprised you got borrow errors. LGTM and no perf impact, thanks! 💯

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 27, 2021

📌 Commit c3a03ae has been approved by Nadrieril

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2021
@bors
Copy link
Collaborator

bors commented Jul 27, 2021

⌛ Testing commit c3a03ae with merge 2faabf5...

@bors
Copy link
Collaborator

bors commented Jul 27, 2021

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 2faabf5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 27, 2021
@bors bors merged commit 2faabf5 into rust-lang:master Jul 27, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 27, 2021
@camelid camelid deleted the check_match-combine-loop branch July 27, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Nadrieril Nadrieril

Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns I-compiletime Issue: Problems and improvements with respect to compile times. 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.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
9 participants
@camelid @rust-highfive @rust-log-analyzer @varkor @jyn514 @rust-timer @bors @Nadrieril @rustbot