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.

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

Shortcuts for min/max on ordinary BTreeMap/BTreeSet iterators #73627

Merged
merged 1 commit into from
Jun 27, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jun 22, 2020

Closes #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 #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).

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Jun 22, 2020
@Mark-Simulacrum
Copy link
Member

Could you provide a summary of the affected impls and which ones are left out of scope?

@ssomers
Copy link
Contributor Author

ssomers commented Jun 26, 2020

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.

@Mark-Simulacrum
Copy link
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 min() and max() not consume the iterator. I personally think that's fine, but I would like to check in with @rust-lang/libs -- do we have guarantees about whether methods like min/max exhaust iterators today (in cases where they can be implemented more efficiently)?

One downside (or upside, I guess) of the current implementation is that you can call min() multiple times and it'll keep returning "the next minimum" which is both nice but also perhaps not what the user expected.

@sfackler
Copy link
Member

In particular, this makes min() and max() not consume the iterator.

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.

@Mark-Simulacrum
Copy link
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+

@bors
Copy link
Contributor

bors commented Jun 26, 2020

📌 Commit 42062a5 has been approved by Mark-Simulacrum

@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 26, 2020
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
@bors bors merged commit dfbba65 into rust-lang:master Jun 27, 2020
@ssomers ssomers deleted the btree_iter_min_max branch June 27, 2020 12:40
@ssomers ssomers changed the title Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators Shortcuts for min/max on ordinary BTreeMap/BTreeSet iterators Jun 27, 2020
@RalfJung
Copy link
Member

RalfJung commented Jul 1, 2020

I think this PR caused a regression: #73915

@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Max & Min on BTreeMap are unexpectedly slow
7 participants