Skip to content

Integrate binary search codes of binary_search_by and partition_point #85406

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

VillSnow
Copy link
Contributor

For now partition_point has own binary search code piece.
It is because binary_search_by had called the comparer more times and the author (=me) wanted to avoid it.

However, now binary_search_by uses the comparer minimum times. (#74024)
So it's time to integrate them.

The appearance of the codes are a bit different but both use completely same logic.

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(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 May 17, 2021
@Folyd
Copy link
Contributor

Folyd commented May 28, 2021

I'd recommend updating the docs of slice::binary_search* to clarify the necessity of partition_point() if they need determinism results. Since the Polkadot had an accident related to my PR recently. See: #85773
What do you think? @VillSnow @m-ou-se

@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 Jun 14, 2021
@JohnTitor
Copy link
Member

I'd recommend updating the docs of slice::binary_search* to clarify the necessity of partition_point() if they need determinism results. Since the Polkadot had an accident related to my PR recently.

Sounds good! But I think it's unrelated to this PR itself and can be done in another PR.

@JohnTitor
Copy link
Member

r? @JohnTitor @bors r+

@bors
Copy link
Collaborator

bors commented Jun 14, 2021

📌 Commit 5db13c5 has been approved by JohnTitor

@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 Jun 14, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 14, 2021
…=JohnTitor

Integrate binary search codes of binary_search_by and partition_point

For now partition_point has own binary search code piece.
It is because binary_search_by had called the comparer more times and the author (=me) wanted to avoid it.

However, now binary_search_by uses the comparer minimum times. (rust-lang#74024)
So it's time to integrate them.

The appearance of the codes are a bit different but both use completely same logic.
@bors
Copy link
Collaborator

bors commented Jun 15, 2021

⌛ Testing commit 5db13c5 with merge 684ca33...

@bors
Copy link
Collaborator

bors commented Jun 16, 2021

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing 684ca33 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2021
@bors bors merged commit 684ca33 into rust-lang:master Jun 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2021
…ochenkov

Prefer `partition_point` to look up assoc items

Since we now have `partition_point` (instead of `equal_range`), I think it's worth trying to use it instead of manually finding it.
`partition_point` uses `binary_search_by` internally (rust-lang#85406) and its performance has been improved (rust-lang#74024), so I guess this will make a performance difference.
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
…ohnTitor

Integrate binary search codes of binary_search_by and partition_point

For now partition_point has own binary search code piece.
It is because binary_search_by had called the comparer more times and the author (=me) wanted to avoid it.

However, now binary_search_by uses the comparer minimum times. (rust-lang#74024)
So it's time to integrate them.

The appearance of the codes are a bit different but both use completely same logic.
# for free to join this conversation on GitHub. Already have an account? # to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants