Have all methods of Filter and FilterMap use internal iteration #58730

timvermeulen · 2019-02-25T16:01:35Z

This PR changes Filter::{next, next_back, count} and FilterMap::{next, next_back} to all use internal iteration. The next and next_back methods are changed to directly forward to try_for_each and try_rfold respectively, using Result as the Try type. I think that's okay? Alternatively, I could change their implementations to use LoopState instead if there's any benefit in doing so.

r? @scottmcm

rust-highfive · 2019-02-25T16:01:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

scottmcm

Thanks for picking this up, @timvermeulen!

The code itself looks right, and it's nice that it's actually shorter, I just have a small side request before merging.

src/libcore/iter/adapters/mod.rs

-        count
+    fn count(self) -> usize {
+        let mut predicate = self.predicate;
+        self.iter.map(|x| predicate(&x) as usize).sum()


timvermeulen · 2019-02-27T11:09:00Z

I added a few benchmarks, of which only bench_filter_chain_count is noticeably faster, as expected:

bench_filter_count              1,240,390 ns/iter (+/- 196,022)  ->  1,255,115 ns/iter (+/- 240,559)
bench_filter_ref_count          1,320,505 ns/iter (+/- 166,123)  ->  1,208,644 ns/iter (+/- 97,721)
bench_filter_chain_count        3,869,760 ns/iter (+/- 676,929)  ->  2,554,825 ns/iter (+/- 395,721)
bench_filter_chain_ref_count    4,198,060 ns/iter (+/- 714,083)  ->  4,207,487 ns/iter (+/- 638,199)

And here are the results of the existing Filter and FilterMap benchmarks:

bench_filter_sum                1,035,545 ns/iter (+/- 273,430)    ->    829,121 ns/iter (+/- 123,512)
bench_filter_ref_sum            1,221,215 ns/iter (+/- 331,032)    ->  1,214,944 ns/iter (+/- 351,951)
bench_filter_chain_sum          1,989,356 ns/iter (+/- 358,755)    ->  2,101,564 ns/iter (+/- 372,251)
bench_filter_chain_ref_sum      5,924,146 ns/iter (+/- 1,753,172)  ->  3,223,435 ns/iter (+/- 594,346)

bench_filter_map_sum              649,279 ns/iter (+/- 117,605)    ->    569,131 ns/iter (+/- 37,360)
bench_filter_map_ref_sum        1,275,035 ns/iter (+/- 184,781)    ->  1,138,938 ns/iter (+/- 35,414)
bench_filter_map_chain_sum      1,456,455 ns/iter (+/- 561,567)    ->  1,222,999 ns/iter (+/- 165,497)
bench_filter_map_chain_ref_sum  5,696,314 ns/iter (+/- 698,883)    ->  4,729,670 ns/iter (+/- 1,519,231)

rust-highfive · 2019-02-27T11:19:00Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:18abb11c:start=1551265483687160989,finish=1551265557035009599,duration=73347848610
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:04:02] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:02] tidy error: /checkout/src/libcore/benches/iter.rs: missing trailing newline
[00:04:04] some tidy checks failed
[00:04:04] 
[00:04:04] 
[00:04:04] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:04] 
[00:04:04] 
[00:04:04] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:04] Build completed unsuccessfully in 0:00:46
[00:04:04] Build completed unsuccessfully in 0:00:46
[00:04:04] Makefile:68: recipe for target 'tidy' failed
[00:04:04] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:015f8318
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Feb 27 11:10:11 UTC 2019
---
travis_time:end:0fd3d0ec:start=1551265812290831832,finish=1551265812295253243,duration=4421411
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:038d9d13
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1220da68
travis_time:start:1220da68
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:11cf80f8
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

scottmcm · 2019-02-27T18:54:14Z

Thanks, @timvermeulen! Great to see the other benchmarks made more representative too 🙂

@bors r+ rollup

bors · 2019-02-27T18:54:15Z

📌 Commit 88bd624 has been approved by scottmcm

bors · 2019-02-27T18:54:35Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Support defining C compatible variadic functions #57760

bors · 2019-02-27T18:54:36Z

📌 Commit 88bd624 has been approved by scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

@scottmcm

…scottmcm Have all methods of Filter and FilterMap use internal iteration This PR changes `Filter::{next, next_back, count}` and `FilterMap::{next, next_back}` to all use internal iteration. The `next` and `next_back` methods are changed to directly forward to `try_for_each` and `try_rfold` respectively, using `Result` as the `Try` type. I think that's okay? Alternatively, I could change their implementations to use `LoopState` instead if there's any benefit in doing so. r? @scottmcm

Rollup of 14 pull requests Successful merges: - #58730 (Have all methods of Filter and FilterMap use internal iteration) - #58780 (ManuallyDrop != MaybeUninit) - #58782 (Replace `s` with `self` in docs for str methods taking self.) - #58785 (allow specifying attributes for tool lints) - #58802 (Ensure `record_layout_for_printing()` is inlined.) - #58821 (Fixed a syntax error in the pin docs) - #58830 (tidy: deny(rust_2018_idioms)) - #58832 (Revert switching to GCP on AppVeyor) - #58833 (tools/rustbook: deny(rust_2018_idioms)) - #58835 (tools/remote-test-{client,server}: deny(rust_2018_idioms)) - #58838 (Fix typo in Vec#resize_with documentation) - #58842 (Forbid duplicating Cargo as a dependency) - #58852 (Update toolchain to build NetBSD release) - #58865 (Fix C-variadic function printing)

Have all methods of Filter and FilterMap use internal iteration

1fd2f16

rust-highfive assigned scottmcm Feb 25, 2019

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

scottmcm requested changes Feb 26, 2019

View reviewed changes

src/libcore/iter/adapters/mod.rs Show resolved Hide resolved

src/libcore/iter/adapters/mod.rs

count

fn count(self) -> usize {

let mut predicate = self.predicate;

self.iter.map(|x| predicate(&x) as usize).sum()

This comment was marked as resolved.

Sign in to view

timvermeulen added 2 commits February 27, 2019 11:44

Add relevant benchmarks

8884771

Improve existing benchmarks to prevent extreme optimizations

ec2e4ba

Add trailing newline

88bd624

scottmcm approved these changes Feb 27, 2019

View reviewed changes

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 27, 2019

Centril mentioned this pull request Feb 28, 2019

Rollup of 8 pull requests #58798

Closed

Centril mentioned this pull request Feb 28, 2019

Rollup of 7 pull requests #58799

Closed

Centril mentioned this pull request Feb 28, 2019

Rollup of 6 pull requests #58801

Closed

GuillaumeGomez mentioned this pull request Feb 28, 2019

Rollup of 6 pull requests #58820

Closed

pietroalbini mentioned this pull request Mar 1, 2019

Rollup of 11 pull requests #58831

Closed

kennytm mentioned this pull request Mar 2, 2019

Rollup of 14 pull requests #58866

Merged

Mark-Simulacrum mentioned this pull request Mar 2, 2019

Rollup of 14 pull requests #58878

Closed

bors merged commit 88bd624 into rust-lang:master Mar 3, 2019

Thinkofname mentioned this pull request Mar 5, 2019

reached the type-length limit while instantiating #58952

Closed

fabianfreyer mentioned this pull request May 27, 2019

Consumers of libjail-rs need to increase type_length_limit fubarnetes/libjail-rs#59

Closed

phyber mentioned this pull request Jun 6, 2019

type_length_limit must be increased phyber/jail_exporter#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have all methods of Filter and FilterMap use internal iteration #58730

Have all methods of Filter and FilterMap use internal iteration #58730

timvermeulen commented Feb 25, 2019

rust-highfive commented Feb 25, 2019

scottmcm left a comment

This comment was marked as resolved.

timvermeulen commented Feb 27, 2019

rust-highfive commented Feb 27, 2019

scottmcm commented Feb 27, 2019 •

edited

Loading

bors commented Feb 27, 2019

bors commented Feb 27, 2019

bors commented Feb 27, 2019

Have all methods of Filter and FilterMap use internal iteration #58730

Have all methods of Filter and FilterMap use internal iteration #58730

Conversation

timvermeulen commented Feb 25, 2019

rust-highfive commented Feb 25, 2019

scottmcm left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

timvermeulen commented Feb 27, 2019

rust-highfive commented Feb 27, 2019

scottmcm commented Feb 27, 2019 • edited Loading

bors commented Feb 27, 2019

bors commented Feb 27, 2019

bors commented Feb 27, 2019

scottmcm commented Feb 27, 2019 •

edited

Loading