[rustc][data_structures] Simplify binary_search_slice. #114152
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @WaffleLapkin (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
Comment on lines
15
to
16
| // at this point start either points at the first entry with equal key | ||
| // or is equal to size in case all elements have smaller keys |
There was a problem hiding this comment.
Suggested change
| // at this point start either points at the first entry with equal key | |
| // or is equal to size in case all elements have smaller keys | |
| // At this point `start` either points at the first entry with equal or greater key | |
| // or is equal to `size` in case all elements have smaller keys |
There was a problem hiding this comment.
thank you for the suggestions!
|
This looks good, modulo the comment nit. |
WaffleLapkin
approved these changes
Jul 29, 2023
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 30, 2023
…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#114129 (Rustdoc small cleanups) - rust-lang#114152 ([rustc][data_structures] Simplify binary_search_slice.) - rust-lang#114222 (Mark `lazy_type_alias` as incomplete) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 18, 2024
…llot [rustc_data_structures] Use partition_point to find slice range end. This PR uses approach introduced in rust-lang#114152 to find the end of the range. It's much easier to understand and reason about invariants of such implementation. Technically it's possible to make it even shorter by returning `&[start..end]` unconditionally because even if searched item is not present in the slice, `start` and `end` would point at the same index, so the range would be empty. The reason I decided not to use this shorter implementation is because it would involve more comparisons in case there are no elements in the slice with key equal to `key`. Also, not that it matters much, but this implementation also improves perf according to the benchmark below: https://gist.github.com/ttsugriy/63c0ed39ae132b131931fa1f8a3dea55 The results on my M1 macbook air are: ``` Running benches/bin_search_slice_benchmark.rs (target/release/deps/bin_search_slice_benchmark-90fa6d68c3bd1298) Benchmarking multiply add/binary_search_slice: Collecting 100 samples in estimated 5.0002 s (1 multiply add/binary_search_slice time: [44.719 ns 44.918 ns 45.158 ns] No change in performance detected. Found 3 outliers among 100 measurements (3.00%) 1 (1.00%) high mild 2 (2.00%) high severe Benchmarking multiply add/binary_search_slice_new: Collecting 100 samples in estimated 5.0001 multiply add/binary_search_slice_new time: [36.955 ns 37.060 ns 37.221 ns] No change in performance detected. Found 7 outliers among 100 measurements (7.00%) 3 (3.00%) high mild 4 (4.00%) high severe ```
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Jan 21, 2024
[rustc_data_structures] Use partition_point to find slice range end. This PR uses approach introduced in rust-lang/rust#114152 to find the end of the range. It's much easier to understand and reason about invariants of such implementation. Technically it's possible to make it even shorter by returning `&[start..end]` unconditionally because even if searched item is not present in the slice, `start` and `end` would point at the same index, so the range would be empty. The reason I decided not to use this shorter implementation is because it would involve more comparisons in case there are no elements in the slice with key equal to `key`. Also, not that it matters much, but this implementation also improves perf according to the benchmark below: https://gist.github.com/ttsugriy/63c0ed39ae132b131931fa1f8a3dea55 The results on my M1 macbook air are: ``` Running benches/bin_search_slice_benchmark.rs (target/release/deps/bin_search_slice_benchmark-90fa6d68c3bd1298) Benchmarking multiply add/binary_search_slice: Collecting 100 samples in estimated 5.0002 s (1 multiply add/binary_search_slice time: [44.719 ns 44.918 ns 45.158 ns] No change in performance detected. Found 3 outliers among 100 measurements (3.00%) 1 (1.00%) high mild 2 (2.00%) high severe Benchmarking multiply add/binary_search_slice_new: Collecting 100 samples in estimated 5.0001 multiply add/binary_search_slice_new time: [36.955 ns 37.060 ns 37.221 ns] No change in performance detected. Found 7 outliers among 100 measurements (7.00%) 3 (3.00%) high mild 4 (4.00%) high severe ```
lnicola
pushed a commit
to lnicola/rust-analyzer
that referenced
this pull request
Apr 7, 2024
[rustc_data_structures] Use partition_point to find slice range end. This PR uses approach introduced in rust-lang/rust#114152 to find the end of the range. It's much easier to understand and reason about invariants of such implementation. Technically it's possible to make it even shorter by returning `&[start..end]` unconditionally because even if searched item is not present in the slice, `start` and `end` would point at the same index, so the range would be empty. The reason I decided not to use this shorter implementation is because it would involve more comparisons in case there are no elements in the slice with key equal to `key`. Also, not that it matters much, but this implementation also improves perf according to the benchmark below: https://gist.github.com/ttsugriy/63c0ed39ae132b131931fa1f8a3dea55 The results on my M1 macbook air are: ``` Running benches/bin_search_slice_benchmark.rs (target/release/deps/bin_search_slice_benchmark-90fa6d68c3bd1298) Benchmarking multiply add/binary_search_slice: Collecting 100 samples in estimated 5.0002 s (1 multiply add/binary_search_slice time: [44.719 ns 44.918 ns 45.158 ns] No change in performance detected. Found 3 outliers among 100 measurements (3.00%) 1 (1.00%) high mild 2 (2.00%) high severe Benchmarking multiply add/binary_search_slice_new: Collecting 100 samples in estimated 5.0001 multiply add/binary_search_slice_new time: [36.955 ns 37.060 ns 37.221 ns] No change in performance detected. Found 7 outliers among 100 measurements (7.00%) 3 (3.00%) high mild 4 (4.00%) high severe ```
RalfJung
pushed a commit
to RalfJung/rust-analyzer
that referenced
this pull request
Apr 27, 2024
[rustc_data_structures] Use partition_point to find slice range end. This PR uses approach introduced in rust-lang/rust#114152 to find the end of the range. It's much easier to understand and reason about invariants of such implementation. Technically it's possible to make it even shorter by returning `&[start..end]` unconditionally because even if searched item is not present in the slice, `start` and `end` would point at the same index, so the range would be empty. The reason I decided not to use this shorter implementation is because it would involve more comparisons in case there are no elements in the slice with key equal to `key`. Also, not that it matters much, but this implementation also improves perf according to the benchmark below: https://gist.github.com/ttsugriy/63c0ed39ae132b131931fa1f8a3dea55 The results on my M1 macbook air are: ``` Running benches/bin_search_slice_benchmark.rs (target/release/deps/bin_search_slice_benchmark-90fa6d68c3bd1298) Benchmarking multiply add/binary_search_slice: Collecting 100 samples in estimated 5.0002 s (1 multiply add/binary_search_slice time: [44.719 ns 44.918 ns 45.158 ns] No change in performance detected. Found 3 outliers among 100 measurements (3.00%) 1 (1.00%) high mild 2 (2.00%) high severe Benchmarking multiply add/binary_search_slice_new: Collecting 100 samples in estimated 5.0001 multiply add/binary_search_slice_new time: [36.955 ns 37.060 ns 37.221 ns] No change in performance detected. Found 7 outliers among 100 measurements (7.00%) 3 (3.00%) high mild 4 (4.00%) high severe ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of using
binary_search_by_key, it's possible to usepartition_pointto find the lower bound. This avoids the need to locate the leftmost matching entry separately.It's also possible to use
partition_pointto find the upper bound, so I plan to send a separate PR for your consideration.