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

Use fold in Iterator::last default implementation #62481

Merged
merged 1 commit into from
Jul 11, 2019

Conversation

czipperz
Copy link
Contributor

@czipperz czipperz commented Jul 7, 2019

We already use it in all the other methods. Consistency + potential perf is a pretty nice win!

@rust-highfive
Copy link
Collaborator

r? @sfackler

(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 Jul 7, 2019
@czipperz
Copy link
Contributor Author

czipperz commented Jul 7, 2019

cc @scottmcm @cuviper from #61726

@czipperz
Copy link
Contributor Author

czipperz commented Jul 7, 2019

So nth doesn't work because Self: Sized is not necessarily fulfilled. Why does try_fold require Self: Sized? The default implementation doesn't use it and it doesn't take self by value. Strange

@czipperz czipperz force-pushed the iterator-last-nth-use-for_each branch from aa0c942 to 23450c3 Compare July 7, 2019 23:57
@cuviper
Copy link
Member

cuviper commented Jul 8, 2019

Regarding try_fold -- methods with generic parameters need Self: Sized for trait object safety.

The last() change looks fine to me.

@czipperz
Copy link
Contributor Author

czipperz commented Jul 8, 2019

Mmh. I didn't think about that. I'll change the title.

@czipperz czipperz changed the title Use for_each and try_fold in Iterator::{last, nth} default implementations Use for_each in Iterator::last default implementation Jul 8, 2019
@scottmcm
Copy link
Member

scottmcm commented Jul 8, 2019

Looks good; thanks for the PR!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 8, 2019

📌 Commit 23450c3da83366c67bda42efb827509bba581df5 has been approved by scottmcm

@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 Jul 8, 2019
@scottmcm scottmcm assigned scottmcm and unassigned sfackler Jul 8, 2019
@scottmcm
Copy link
Member

scottmcm commented Jul 8, 2019

@bors r-

Actually, @czipperz, can you squash the commits here please? (Or just force-push to
53ce67c.) Let's not merge in history that immediately reverts itself :)

@bors 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 Jul 8, 2019
@timvermeulen
Copy link
Contributor

It shouldn't matter, but you may as well use fold here?

@llogiq
Copy link
Contributor

llogiq commented Jul 8, 2019

I just did a quick benchmark and self.fold(None, move |_, x| Some(x)) was around 10% faster, besides being shorter and IMHO more readable.

@czipperz
Copy link
Contributor Author

czipperz commented Jul 9, 2019

I'm surprised it is that much faster wow! Could you share your benchmark code / data?

@llogiq
Copy link
Contributor

llogiq commented Jul 9, 2019

#![feature(test)]

extern crate test;
use test::{black_box, Bencher};

#[bench]
fn bench_for_each(b: &mut Bencher) {
    fn last<I: Iterator>(i: I) -> Option<<I as Iterator>::Item> where I: Sized {
        let mut last = None;
        i.for_each(|x| last = Some(x));
        last
    }
    let v = vec![0usize; 1024];
    b.iter(|| { last(black_box(v.iter())) })
}

#[bench]
fn bench_fold(b: &mut Bencher) {
    fn last<I: Iterator>(i: I) -> Option<<I as Iterator>::Item> where I: Sized {
        i.fold(None, move |_, x| Some(x))
    }
    let v = vec![0usize; 1024];
    b.iter(|| { last(black_box(v.iter())) })
}

Oops, I forgot running the benchmark with -C opt_level=3. The for_each version gets zero runtime with that, which kind of invalidates the benchmark, though. More research is needed.

@czipperz
Copy link
Contributor Author

czipperz commented Jul 9, 2019

I do think fold is clearer though

@czipperz czipperz changed the title Use for_each in Iterator::last default implementation Use fold in Iterator::last default implementation Jul 10, 2019
Replace last impl with fold
@czipperz czipperz force-pushed the iterator-last-nth-use-for_each branch from 30e6ec3 to 76a8bc2 Compare July 10, 2019 04:27
@czipperz
Copy link
Contributor Author

@scottmcm squashed it up

@scottmcm
Copy link
Member

Thanks, @czipperz!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 10, 2019

📌 Commit 76a8bc2 has been approved by scottmcm

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 10, 2019
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 10, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 10, 2019
…each, r=scottmcm

Use `fold` in `Iterator::last` default implementation

We already use it in all the other methods.  Consistency + potential perf is a pretty nice win!
bors added a commit that referenced this pull request Jul 10, 2019
Rollup of 5 pull requests

Successful merges:

 - #62275 (rustc_mir: treat DropAndReplace as Drop + Assign in qualify_consts.)
 - #62465 (Sometimes generate storage statements for temporaries with type `!`)
 - #62481 (Use `fold` in `Iterator::last` default implementation)
 - #62493 (#62357: doc(ptr): add example for {read,write}_unaligned)
 - #62532 (Some more cleanups to syntax::print)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Jul 10, 2019
Rollup of 5 pull requests

Successful merges:

 - #62275 (rustc_mir: treat DropAndReplace as Drop + Assign in qualify_consts.)
 - #62465 (Sometimes generate storage statements for temporaries with type `!`)
 - #62481 (Use `fold` in `Iterator::last` default implementation)
 - #62493 (#62357: doc(ptr): add example for {read,write}_unaligned)
 - #62532 (Some more cleanups to syntax::print)

Failed merges:

r? @ghost
@bors bors merged commit 76a8bc2 into rust-lang:master Jul 11, 2019
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.

8 participants