Let `try_collect` take advantage of `try_fold` overrides #94115

scottmcm · 2022-02-18T07:39:46Z

No public API changes.

With this change, try_collect (#94047) is no longer going through the impl Iterator for &mut impl Iterator, and thus will be able to use try_fold overrides instead of being forced through next for every element.

Here's the test added, to see that it fails before this PR (once a new enough nightly is out): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=462f2896f2fed2c238ee63ca1a7e7c56

This might as well go to the same person as my last try_process PR (#93572), so
r? @yaahc

yaahc · 2022-02-18T19:34:52Z

Took me a second to figure out why you didn't have to edit the try_collect impl body at all but I figured it out. Looks good.

@bors r+

bors · 2022-02-18T19:34:54Z

📌 Commit 2cbba35793b4cee97f0d29b3ace18bdb77d6426c has been approved by yaahc

the8472 · 2022-02-18T20:25:27Z

I have concerns about whether this is valid for the unsafe impl SourceIter for GenericShunt impl. I'll have to think about this. Current uses shouldn't be affected but it could be used incorrectly.

the8472 · 2022-02-18T20:32:29Z

Hope you don't mind. @bors r-

the8472 · 2022-02-18T20:49:22Z

Indeed, this is incompatible with this safety requirement

rust/library/core/src/iter/adapters/mod.rs

Lines 103 to 108 in b8c56fa

    
               /// # Safety 
        
               /// 
        
               /// Implementations of must return the same mutable reference for their lifetime, unless 
        
               /// replaced by a caller. 
        
               /// Callers may only replace the reference when they stopped iteration and drop the 
        
               /// iterator pipeline after extracting the source.

You cannot be sure to have dropped the iterator pipeline (that is, including the source) if it can take a &mut iter.

The unsafe traits are used to optimize iterators such as this one to run in place

vec.into_iter().map(Foo::something_returning_result).collect::<Result<Vec<_>, _>>()

scottmcm · 2022-02-18T21:20:30Z

Thanks, @the8472! Definitely don't mind an r- for unsoundness; exactly the opposite.

Does anyone have thoughts on the best way for me to move forward, here?

Should I make a separate ShuntMut iterator and duplicate most of the stuff?

the8472 · 2022-02-18T21:29:00Z

Another option is to use iter.by_ref().try_process() which is safe because impl Iterator for &mut I does not implement those unsafe traits. But since you're making this change for performance rasons (to get try_fold) that would only help after #82185 or a subset thereof (passing through try_fold).

the8472 · 2022-02-18T23:26:02Z

You could also keep using GenericShunt and specialize it for GenericShunt<I: &mut Iterator + Sized> so it can keep using try_fold. That's a narrower specialization compared to #82185 and so hopefully wouldn't suffer from much negative performance.

scottmcm · 2022-02-20T18:38:43Z

Rather than use specialization, I added a pub(crate) wrapper that works like .by_ref() but requires Sized so it forwards try_fold & fold.

That seems most obviously correct to me, is potentially re-usable, and would be an easy thing to remove if specialization ever removes the suboptimality from by_ref.

the8472

Looks ok from the safety perspective now. The new adapter doesn't match other adapters in usual optimizations that are applied (inline annotations, TrustedLen and similar traits).
Some microbenchmarking could help, but that can be done in a separate PR.

Additionally an internal comment pointing from impl Iterator for &mut Iterator to the adapter could be useful to maintain awareness if someone tries to optimize or use that in the future.

bors · 2022-03-10T05:08:43Z

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

Without this change it's going through `&mut impl Iterator`, which handles `?Sized` and thus currently can't forward generics. Here's the test added, to see that it fails before this PR (once a new enough nightly is out): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=462f2896f2fed2c238ee63ca1a7e7c56

yaahc · 2022-03-18T18:08:26Z

lgtm, tyty @scottmcm and @the8472

@bors r+

bors · 2022-03-18T18:08:27Z

📌 Commit 7ef74bc has been approved by yaahc

Let `try_collect` take advantage of `try_fold` overrides No public API changes. With this change, `try_collect` (rust-lang#94047) is no longer going through the `impl Iterator for &mut impl Iterator`, and thus will be able to use `try_fold` overrides instead of being forced through `next` for every element. Here's the test added, to see that it fails before this PR (once a new enough nightly is out): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=462f2896f2fed2c238ee63ca1a7e7c56 This might as well go to the same person as my last `try_process` PR (rust-lang#93572), so r? `@yaahc`

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#94115 (Let `try_collect` take advantage of `try_fold` overrides) - rust-lang#94295 (Always evaluate all cfg predicate in all() and any()) - rust-lang#94848 (Compare installed browser-ui-test version to the one used in CI) - rust-lang#94993 (Add test for >65535 hashes in lexing raw string) - rust-lang#95017 (Derive Eq for std::cmp::Ordering, instead of using manual impl.) - rust-lang#95058 (Add use of bool::then in sys/unix/process) - rust-lang#95083 (Document that `Option<extern "abi" fn>` discriminant elision applies for any ABI) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

scottmcm added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 18, 2022

rust-highfive assigned yaahc Feb 18, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2022

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 Feb 18, 2022

bors added 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 18, 2022

scottmcm changed the title ~~iter::try_process doesn't need ownership, so take &mut instead~~ Let try_collect take advantage of try_fold overrides Feb 20, 2022

scottmcm force-pushed the iter-process-by-ref branch from 2cbba35 to ee50c35 Compare February 20, 2022 18:34

scottmcm 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 Feb 20, 2022

the8472 reviewed Mar 2, 2022

View reviewed changes

scottmcm force-pushed the iter-process-by-ref branch from ee50c35 to 7ef74bc Compare March 10, 2022 08:16

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 Mar 18, 2022

Dylan-DPC mentioned this pull request Mar 18, 2022

Rollup of 6 pull requests #95084

Closed

matthiaskrgr mentioned this pull request Mar 18, 2022

Rollup of 7 pull requests #95090

Merged

bors merged commit c183d4a into rust-lang:master Mar 19, 2022

rustbot added this to the 1.61.0 milestone Mar 19, 2022

scottmcm deleted the iter-process-by-ref branch March 19, 2022 01:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let `try_collect` take advantage of `try_fold` overrides #94115

Let `try_collect` take advantage of `try_fold` overrides #94115

scottmcm commented Feb 18, 2022

yaahc commented Feb 18, 2022

bors commented Feb 18, 2022

the8472 commented Feb 18, 2022 •

edited

Loading

the8472 commented Feb 18, 2022

the8472 commented Feb 18, 2022

scottmcm commented Feb 18, 2022

the8472 commented Feb 18, 2022

the8472 commented Feb 18, 2022

scottmcm commented Feb 20, 2022

the8472 left a comment

bors commented Mar 10, 2022

yaahc commented Mar 18, 2022

bors commented Mar 18, 2022

Let try_collect take advantage of try_fold overrides #94115

Let try_collect take advantage of try_fold overrides #94115

Conversation

scottmcm commented Feb 18, 2022

yaahc commented Feb 18, 2022

bors commented Feb 18, 2022

the8472 commented Feb 18, 2022 • edited Loading

the8472 commented Feb 18, 2022

the8472 commented Feb 18, 2022

scottmcm commented Feb 18, 2022

the8472 commented Feb 18, 2022

the8472 commented Feb 18, 2022

scottmcm commented Feb 20, 2022

the8472 left a comment

Choose a reason for hiding this comment

bors commented Mar 10, 2022

yaahc commented Mar 18, 2022

bors commented Mar 18, 2022

Let `try_collect` take advantage of `try_fold` overrides #94115

Let `try_collect` take advantage of `try_fold` overrides #94115

the8472 commented Feb 18, 2022 •

edited

Loading