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

Improve get_by_key_enumerated more #86429

Merged
merged 3 commits into from
Jul 24, 2021
Merged

Improve get_by_key_enumerated more #86429

merged 3 commits into from
Jul 24, 2021

Conversation

JohnTitor
Copy link
Member

Follow-up of #86392, this applies the suggestions by @m-ou-se.

r? @m-ou-se

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2021
@JohnTitor

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 18, 2021
@bors

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (172d0d098473f7e9b4ee053d7ed424405c064d3a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 18, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 18, 2021

This still requires finding the upper bound/amount of items before returning the iterator. That's needed because the key (of type &Q) doesn't live long enough to return it. But since get_by_key_enumerated is only ever used by get_by_key, and get_by_key is only ever used with K = Symbol (which is Copy and even smaller than a reference), maybe it's best to remove the Q type, and just take a K by value. Then you can return the .take_while() iterator directly, without .count()'ing it.

@JohnTitor
Copy link
Member Author

So, you mean making SortedIndexMultiMap Symbol-specific? If so, wouldn't it need us to move out SortedIndexMultiMap to somewhere?

@m-ou-se
Copy link
Member

m-ou-se commented Jun 18, 2021

No, I'm suggesting just taking the K by value, instead of borrowing a Q. Then you can just move it into the closure, and return the .take_while() directly.

@JohnTitor
Copy link
Member Author

Ah, that makes sense! Applied the suggestion via d731d61.

@JohnTitor

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 19, 2021
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (a683ef3cff6fb2682ca8fbed8aa293340539643d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 19, 2021
@m-ou-se m-ou-se added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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 Jun 19, 2021
m-ou-se
m-ou-se reviewed Jun 19, 2021
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Nice.

(You still need to update the tests.)

Since I'm not on the compiler team or compiler-contributors, assigning it to someone on the compiler team for final approval:

r? @matthewjasper

compiler/rustc_data_structures/src/sorted_map/index_map.rs Outdated Show resolved Hide resolved
compiler/rustc_data_structures/src/sorted_map/index_map.rs Outdated Show resolved Hide resolved
@m-ou-se m-ou-se removed their assignment Jun 19, 2021
@JohnTitor JohnTitor force-pushed the get-by-key-enum-part-2 branch from d731d61 to 55974fa Compare June 20, 2021 01:46
@JohnTitor JohnTitor 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 Jun 20, 2021
@JohnTitor JohnTitor force-pushed the get-by-key-enum-part-2 branch from 55974fa to edc012a Compare June 21, 2021 21:30
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JohnTitor JohnTitor force-pushed the get-by-key-enum-part-2 branch from edc012a to 702ca32 Compare June 21, 2021 21:45
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2021
@bors
Copy link
Contributor

bors commented Jul 23, 2021

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

@JohnTitor JohnTitor force-pushed the get-by-key-enum-part-2 branch from 702ca32 to 7cc7e27 Compare July 23, 2021 09:06
@JohnTitor JohnTitor force-pushed the get-by-key-enum-part-2 branch from 7cc7e27 to 6761826 Compare July 23, 2021 09:08
@JohnTitor
Copy link
Member Author

r? @oli-obk for re-assigning (according to the GitHub reviewer suggestions)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 23, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 23, 2021

📌 Commit 6761826 has been approved by oli-obk

@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 23, 2021
@bors
Copy link
Contributor

bors commented Jul 23, 2021

⌛ Testing commit 6761826 with merge 3b4a0df...

@bors
Copy link
Contributor

bors commented Jul 24, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 3b4a0df to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 24, 2021
@bors bors merged commit 3b4a0df into rust-lang:master Jul 24, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 24, 2021
@JohnTitor JohnTitor deleted the get-by-key-enum-part-2 branch July 24, 2021 04:48
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

Assignees

@oli-obk oli-obk

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.55.0
Development

Successfully merging this pull request may close these issues.
10 participants
@JohnTitor @rust-timer @bors @m-ou-se @rust-log-analyzer @oli-obk @JohnCSimon @rust-highfive @matthewjasper @rustbot