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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement nth_back for slice::{Iter, IterMut} #60772

Merged
merged 1 commit into from Jun 20, 2019

Conversation

Projects
None yet
9 participants
@timvermeulen
Copy link
Contributor

commented May 12, 2019

Part of #54054.

I implemented nth_back as straightforwardly as I could, and then slightly changed nth to match nth_back. I believe I did so correctly, but please double-check 馃檪

I also added the helper methods zst_shrink, next_unchecked, and next_back_unchecked to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the is_empty! and len! macros which sole purpose seems to be inlining, according to the comment right above them, but the is_empty and len methods are already marked with #[inline(always)]. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

// end of the iterator backwards by `n`.
#[inline(always)]
fn zst_shrink(&mut self, n: isize) {
self.end = (self.end as * $raw_mut u8).wrapping_offset(-n) as * $raw_mut T;

This comment has been minimized.

Copy link
@RalfJung

RalfJung May 13, 2019

Member

Maybe add a debug_assert making sure mem::size_of::<T>() == 0?

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen May 13, 2019

Author Contributor

I didn't do this as not to affect the performance of debug builds, although I'm not opposed to adding it. Either way, this function should probably be unsafe?

accum = f(accum, self.next_unchecked())?;
accum = f(accum, self.next_unchecked())?;
accum = f(accum, self.next_unchecked())?;
accum = f(accum, self.next_unchecked())?;

This comment has been minimized.

Copy link
@RalfJung

RalfJung May 13, 2019

Member

Last time I touched these, even introducing inline(always) functions in some of these places had significant impact on performance. This should get a perf run, and ideally also some manual benchmarking.

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen May 13, 2019

Author Contributor

Yikes, yep, benchmarks I just ran confirm this. So are they not actually being inlined, or what's going on?

This comment has been minimized.

Copy link
@RalfJung

RalfJung May 13, 2019

Member

So we don't need to bother perfbot then?^^

So are they not actually being inlined, or what's going on?

Your guess is as good as mine. One hypothesis we had last time was that inlining happens after some other passes -- so manually inlined code basically is already present earlier in the pipeline and gets simplified better.

You could try to compile your benchmark to LLVM IR with the old and new version of libstd and compare. You'll see that the function did get inlined, but the IR is still vastly different.

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen May 13, 2019

Author Contributor

It's unfortunate that functions apparently aren't zero-cost here, but I guess it is what it is.

I turned next_unchecked and next_back_unchecked into macros and that seems to have fixed all the regressions. 馃檪 It's hard to say for sure though, every time I run the benchmarks there are a few that change by up to 5% or so in either direction, although it's not always the same ones. It seems like they're just not very stable.

@RalfJung

This comment has been minimized.

Copy link
Member

commented May 13, 2019

For perf:
@bors try

I noticed the is_empty! and len! macros which sole purpose seems to be inlining, according to the comment right above them, but the is_empty and len methods are already marked with #[inline(always)]. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.

When I tried, this made a difference. See in particular the discussion starting here.

@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

鈱涳笍 Trying commit dde6587 with merge a7f658d...

bors added a commit that referenced this pull request May 13, 2019

Auto merge of #60772 - timvermeulen:slice_iter_nth_back, r=<try>
Implement nth_back for slice::{Iter, IterMut}

Part of #54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 馃檪

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

鈽锔 Try build successful - checks-travis
Build commit: a7f658d

@timvermeulen timvermeulen force-pushed the timvermeulen:slice_iter_nth_back branch from dde6587 to a89a110 May 13, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

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:0545aa4a:start=1557764063421599771,finish=1557764259021378718,duration=195599778947
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:04:21] tidy error: /checkout/src/libcore/slice/mod.rs:3030: trailing whitespace
[00:04:26] some tidy checks failed
[00:04:26] 
[00:04:26] 
[00:04:26] 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:26] 
[00:04:26] 
[00:04:26] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:26] Build completed unsuccessfully in 0:01:12
[00:04:26] Build completed unsuccessfully in 0:01:12
[00:04:26] make: *** [tidy] Error 1
[00:04:26] Makefile:67: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:268c05cc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon May 13 16:22:14 UTC 2019
---
travis_time:end:12cb7b00:start=1557764535551006343,finish=1557764535556284451,duration=5278108
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:014669a0
$ 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:10974a64
travis_time:start:10974a64
$ 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:13c8870b
$ 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)

@timvermeulen timvermeulen force-pushed the timvermeulen:slice_iter_nth_back branch from a89a110 to 3834d0e May 13, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@RalfJung Thanks! That's very helpful. Looks like we can't get rid of the macros yet, then.

@RalfJung

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Let's see what perf has to say, then.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

鈱涳笍 Trying commit 3834d0e with merge e89b9fd...

bors added a commit that referenced this pull request May 13, 2019

Auto merge of #60772 - timvermeulen:slice_iter_nth_back, r=<try>
Implement nth_back for slice::{Iter, IterMut}

Part of #54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 馃檪

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

鈽锔 Try build successful - checks-travis
Build commit: e89b9fd

@RalfJung

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 14, 2019

Success: Queued e89b9fd with parent a9ec99f, comparison URL.

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

It's not clear to me whether the 0.8% change to clap-rs-debug is an actual regression, or just noise... It seems like a slight regression overall if I naively add up all the percentages. Maybe I should turn zst_shrink into a macro, too? That's the only possible reason for regressions that I can see here.

@timvermeulen timvermeulen force-pushed the timvermeulen:slice_iter_nth_back branch from 3834d0e to 97a6c93 May 24, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

I also made zst_shrink into a macro, in order to make all code equivalent to the code on master after all macros are expanded.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@timvermeulen: 馃攽 Insufficient privileges: not in try users

@RalfJung

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

鈱涳笍 Trying commit 97a6c93 with merge 01fe51e...

bors added a commit that referenced this pull request May 25, 2019

Auto merge of #60772 - timvermeulen:slice_iter_nth_back, r=<try>
Implement nth_back for slice::{Iter, IterMut}

Part of #54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 馃檪

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

鈽锔 Try build successful - checks-travis
Build commit: 01fe51e

@RalfJung

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 25, 2019

Success: Queued 01fe51e with parent 315ab95, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 25, 2019

Finished benchmarking try commit 01fe51e: comparison url

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

This looks better!

@Centril

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Triage: Waiting for review by @rust-lang/libs

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Looks good, thanks @timvermeulen!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

馃搶 Commit 97a6c93 has been approved by scottmcm

Centril added a commit to Centril/rust that referenced this pull request Jun 20, 2019

Rollup merge of rust-lang#60772 - timvermeulen:slice_iter_nth_back, r鈥
鈥=scottmcm

Implement nth_back for slice::{Iter, IterMut}

Part of rust-lang#54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 馃檪

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@bors rollup-
due to possible perf impact

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Well that didn't do anything. And it's already been rolled up anyway... whatever^^

bors added a commit that referenced this pull request Jun 20, 2019

Auto merge of #61983 - Centril:rollup-wnfo07y, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #60454 (Add custom nth_back to Skip)
 - #60772 (Implement nth_back for slice::{Iter, IterMut})
 - #61782 (suggest tuple struct syntax)
 - #61968 (rustc: disallow cloning HIR nodes.)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

鈱涳笍 Testing commit 97a6c93 with merge 4fb77a0...

@bors bors merged commit 97a6c93 into rust-lang:master Jun 20, 2019

1 of 2 checks passed

homu Testing commit 97a6c932e02fcd7f55fdad9aef76c5619f91f481 with merge 4fb77a0398d0339f35f1b18595b375428babd431...
Details
Travis CI - Pull Request Build Passed
Details

@timvermeulen timvermeulen deleted the timvermeulen:slice_iter_nth_back branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.