Skip to content

Combine two loops in check_match#80367

Merged
bors merged 1 commit intorust-lang:masterfrom
camelid:check_match-combine-loop
Jul 27, 2021
Merged

Combine two loops in check_match#80367
bors merged 1 commit intorust-lang:masterfrom
camelid:check_match-combine-loop

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

rust-highfive commented Dec 25, 2020

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

rust-timer commented Jul 23, 2021

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

rust-timer commented Jul 23, 2021

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

rust-timer commented Jul 23, 2021

Queued cc556f91642435dd5ba31f27af2d573b5f9e1526 with parent 0443424, future comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Jul 23, 2021

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

Nadrieril commented Jul 27, 2021

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