Shortcuts for min/max on ordinary BTreeMap/BTreeSet iterators#73627
Merged
bors merged 1 commit intorust-lang:masterfrom Jun 27, 2020
ssomers:btree_iter_min_max
Merged
Shortcuts for min/max on ordinary BTreeMap/BTreeSet iterators#73627bors merged 1 commit intorust-lang:masterfrom ssomers:btree_iter_min_max
bors merged 1 commit intorust-lang:masterfrom
ssomers:btree_iter_min_max
Conversation
Contributor
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
ssomers
commented
Jun 23, 2020
Member
|
Could you provide a summary of the affected impls and which ones are left out of scope? |
Contributor
Author
|
I couldn't come up with a reasonable reason to leave some of them out, so added some more. Also straightened out the test code. |
Member
|
Okay, that set of iterators does look correct to me. However, I think the implementation here is a bit wrong if we want to avoid a behavior change. In particular, this makes One downside (or upside, I guess) of the current implementation is that you can call |
Member
For a generic iterator, we do need min() and max() to advance past every element of the iterator, but since these iterators are specific ones where iteration does not have a side effect the change isn't observable. min() and max() consume the iterator by value, so you can't call them multiple times. |
Member
|
Ah for some reason I thought by_ref() let you magically have ownership but that doesn't make any sense now that I think about it some more. Yes, this seems fine then. @bors r+ |
Collaborator
|
📌 Commit 42062a5 has been approved by |
bors
added
S-waiting-on-bors
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 26, 2020
…Simulacrum Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators Closes rust-lang#59947: a performance tweak that might benefit some. Optimizes `min` and `max ` on all btree double-ended iterators that do not drop, i.e. the iterators created by: - `BTreeMap::iter` - `BTreeMap::iter_mut` - `BTreeMap::keys` and `BTreeSet::iter` - `BTreeMap::range` and `BTreeSet::range` - `BTreeMap::range_mut` Also in these (currently) single-ended iterators, but obviously for `min` only: - `BTreeSet::difference` - `BTreeSet::intersection` - `BTreeSet::symmetric_difference` - `BTreeSet::union` Did not do this in iterators created by `into_iter` to preserve drop order, as outlined in rust-lang#62316. Did not do this in iterators created by `drain_filter`, possibly to preserve drop order, possibly to preserve predicate invocation, mostly to not have to think about it too hard (I guess maybe it wouldn't be a change for `min`, which is the only shortcut possible in this single-ended iterator).
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 26, 2020
…Simulacrum Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators Closes rust-lang#59947: a performance tweak that might benefit some. Optimizes `min` and `max ` on all btree double-ended iterators that do not drop, i.e. the iterators created by: - `BTreeMap::iter` - `BTreeMap::iter_mut` - `BTreeMap::keys` and `BTreeSet::iter` - `BTreeMap::range` and `BTreeSet::range` - `BTreeMap::range_mut` Also in these (currently) single-ended iterators, but obviously for `min` only: - `BTreeSet::difference` - `BTreeSet::intersection` - `BTreeSet::symmetric_difference` - `BTreeSet::union` Did not do this in iterators created by `into_iter` to preserve drop order, as outlined in rust-lang#62316. Did not do this in iterators created by `drain_filter`, possibly to preserve drop order, possibly to preserve predicate invocation, mostly to not have to think about it too hard (I guess maybe it wouldn't be a change for `min`, which is the only shortcut possible in this single-ended iterator).
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 26, 2020
…Simulacrum Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators Closes rust-lang#59947: a performance tweak that might benefit some. Optimizes `min` and `max ` on all btree double-ended iterators that do not drop, i.e. the iterators created by: - `BTreeMap::iter` - `BTreeMap::iter_mut` - `BTreeMap::keys` and `BTreeSet::iter` - `BTreeMap::range` and `BTreeSet::range` - `BTreeMap::range_mut` Also in these (currently) single-ended iterators, but obviously for `min` only: - `BTreeSet::difference` - `BTreeSet::intersection` - `BTreeSet::symmetric_difference` - `BTreeSet::union` Did not do this in iterators created by `into_iter` to preserve drop order, as outlined in rust-lang#62316. Did not do this in iterators created by `drain_filter`, possibly to preserve drop order, possibly to preserve predicate invocation, mostly to not have to think about it too hard (I guess maybe it wouldn't be a change for `min`, which is the only shortcut possible in this single-ended iterator).
This was referenced Jun 26, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 27, 2020
…arth Rollup of 12 pull requests Successful merges: - rust-lang#72771 (Warn if linking to a private item) - rust-lang#72937 (Fortanix SGX target libunwind build process changes) - rust-lang#73485 (Perform obligation deduplication to avoid buggy `ExistentialMismatch`) - rust-lang#73529 (Add liballoc impl SpecFromElem for i8) - rust-lang#73579 (add missing doc links) - rust-lang#73627 (Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators) - rust-lang#73691 (Bootstrap: detect Windows based on sys.platform) - rust-lang#73694 (Document the Self keyword) - rust-lang#73718 (Document the super keyword) - rust-lang#73728 (Document some invariants correctly/more) - rust-lang#73738 (Remove irrelevant comment) - rust-lang#73765 (Remove blank line) Failed merges: r? @ghost
ssomers
changed the title
Member
|
I think this PR caused a regression: #73915 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Closes #59947: a performance tweak that might benefit some. Optimizes
minandmaxon all btree double-ended iterators that do not drop, i.e. the iterators created by:BTreeMap::iterBTreeMap::iter_mutBTreeMap::keysandBTreeSet::iterBTreeMap::rangeandBTreeSet::rangeBTreeMap::range_mutAlso in these (currently) single-ended iterators, but obviously for
minonly:BTreeSet::differenceBTreeSet::intersectionBTreeSet::symmetric_differenceBTreeSet::unionDid not do this in iterators created by
into_iterto preserve drop order, as outlined in #62316.Did not do this in iterators created by
drain_filter, possibly to preserve drop order, possibly to preserve predicate invocation, mostly to not have to think about it too hard (I guess maybe it wouldn't be a change formin, which is the only shortcut possible in this single-ended iterator).