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

implement advance_(back_)_by on more iterators #87091

Merged
merged 3 commits into from
Oct 4, 2021

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Jul 12, 2021

Add more efficient, non-default implementations for feature(iter_advance_by) (#77404) on more iterators and adapters.

This PR only contains implementations where skipping over items doesn't elide any observable side-effects such as user-provided closures or clone() functions. I'll put those in a separate PR.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 12, 2021
@the8472
Copy link
Member Author

the8472 commented Jul 12, 2021

@timvermeulen I changed the API contract of a bit. Do they look ok to you?

Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a couple of comments, let me know what you think about them

library/alloc/src/vec/into_iter.rs Show resolved Hide resolved
library/alloc/src/vec/into_iter.rs Show resolved Hide resolved
library/core/src/iter/adapters/flatten.rs Outdated Show resolved Hide resolved
library/core/src/iter/adapters/skip.rs Outdated Show resolved Hide resolved
@the8472
Copy link
Member Author

the8472 commented Jul 13, 2021

Thanks for the reviews @pickfire @SkiFire13 I have closed the conversation for the issues that I think should be addressed now. I have completely rewritten Skip::advance_by so you might want to give it another look.

The fixes are in separate commits for easier review.

@the8472
Copy link
Member Author

the8472 commented Jul 14, 2021

I ran some of the core benchmarks, decent improvements for the .cycle().skip() case.

# RUSTFLAGS="-Ccodegen-units=1 -Copt-level=3" taskset -c 5 schedtool -B -e ./x.py bench --stage 0 library/core/ --test-args "cycle"

 iter::bench_cycle_skip_take_ref_sum          1,837,376         1,110,352              -727,024  -39.57%   x 1.65 
 iter::bench_cycle_skip_take_sum              1,297,196         525,064                -772,132  -59.52%   x 2.47 
 iter::bench_cycle_take_ref_sum               1,096,240         1,093,491                -2,749   -0.25%   x 1.00 
 iter::bench_cycle_take_skip_ref_sum          588,162           717,214                 129,052   21.94%   x 0.82 
 iter::bench_cycle_take_skip_sum              550,002           474,185                 -75,817  -13.78%   x 1.16 
 iter::bench_cycle_take_sum                   524,600           448,794                 -75,806  -14.45%   x 1.17 
 iter::bench_skip_cycle_skip_zip_add_ref_sum  5,336,791         3,761,926            -1,574,865  -29.51%   x 1.42 
 iter::bench_skip_cycle_skip_zip_add_sum      4,075,445         4,038,599               -36,846   -0.90%   x 1.01 

# RUSTFLAGS="-Ccodegen-units=1 -Copt-level=2 -Ctarget-cpu=native" taskset -c 5 schedtool -B -e ./x.py bench --stage 0 library/core/ --test-args "cycle"

 iter::bench_cycle_skip_take_ref_sum          1,396,256         1,087,191              -309,065  -22.14%   x 1.28 
 iter::bench_cycle_skip_take_sum              1,260,624         474,665                -785,959  -62.35%   x 2.66 
 iter::bench_cycle_take_ref_sum               1,095,467         1,094,409                -1,058   -0.10%   x 1.00 
 iter::bench_cycle_take_skip_ref_sum          1,058,240         1,008,308               -49,932   -4.72%   x 1.05 
 iter::bench_cycle_take_skip_sum              496,065           448,904                 -47,161   -9.51%   x 1.11 
 iter::bench_cycle_take_sum                   469,290           466,544                  -2,746   -0.59%   x 1.01 
 iter::bench_skip_cycle_skip_zip_add_ref_sum  3,453,381         3,737,632               284,251    8.23%   x 0.92 
 iter::bench_skip_cycle_skip_zip_add_sum      3,179,488         3,472,014               292,526    9.20%   x 0.92 

The zip case is interesting

    (0..100000).skip(100).cycle().skip(100)
      .zip((0..100000).cycle().skip(10))
      .map(|(a,b)| a+b)
      .skip(100000)
      .take(1000000)
      .sum()

The outer skip will call nth but that can't benefit from advance_by because as long as we have to preserve side-effects map has to use the default implementation for that. But the zipped iterators contain Skip too, which do benefit from advance_by. But that seems very opt-level-dependent.

@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 25, 2021
@bors
Copy link
Contributor

bors commented Aug 5, 2021

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 14, 2021

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

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021
@bors
Copy link
Contributor

bors commented Sep 22, 2021

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 30, 2021

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

@joshtriplett
Copy link
Member

This looks reasonable to me.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2021

📌 Commit ffd7ade has been approved by joshtriplett

@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 Oct 4, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…shtriplett

implement advance_(back_)_by on more iterators

Add more efficient, non-default implementations for `feature(iter_advance_by)` (rust-lang#77404) on more iterators and adapters.

This PR only contains implementations where skipping over items doesn't elide any observable side-effects such as user-provided closures or `clone()` functions. I'll put those in a separate PR.
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…shtriplett

implement advance_(back_)_by on more iterators

Add more efficient, non-default implementations for `feature(iter_advance_by)` (rust-lang#77404) on more iterators and adapters.

This PR only contains implementations where skipping over items doesn't elide any observable side-effects such as user-provided closures or `clone()` functions. I'll put those in a separate PR.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2021
…ingjubilee

Rollup of 13 pull requests

Successful merges:

 - rust-lang#83655 ([aarch64] add target feature outline-atomics)
 - rust-lang#87091 (implement advance_(back_)_by on more iterators)
 - rust-lang#88451 (Fix an ICE caused by type mismatch errors being ignored)
 - rust-lang#88452 (VecDeque: improve performance for From<[T; N]>)
 - rust-lang#89400 (Improve wording of `map_or_else` docs)
 - rust-lang#89407 (Recommend running `cargo clean` in E0514 output)
 - rust-lang#89443 (Include the length in BTree hashes)
 - rust-lang#89444 (rustdoc: use slice::contains instead of open-coding it)
 - rust-lang#89447 (Improve error message for missing angle brackets in `[_]::method`)
 - rust-lang#89453 (Consistently use 'supertrait'.)
 - rust-lang#89483 (Practice diagnostic message convention)
 - rust-lang#89500 (Fix ICE with buffered lint referring to AST node deleted by everybody_loops)
 - rust-lang#89508 (Stabilize `const_panic`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ca8a108 into rust-lang:master Oct 4, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 4, 2021
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants