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 TrustedRandomAccess for slice::{Chunks, ChunksMut, Windows} #47142

Merged
merged 4 commits into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@sdroege
Contributor

sdroege commented Jan 2, 2018

As suggested by @bluss in #47115 (comment)

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jan 2, 2018

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (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.

Collaborator

rust-highfive commented Jan 2, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (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.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 3, 2018

Member

Thanks!

@bors r+

Member

kennytm commented Jan 3, 2018

Thanks!

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

📌 Commit 43291c6 has been approved by kennytm

Contributor

bors commented Jan 3, 2018

📌 Commit 43291c6 has been approved by kennytm

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 3, 2018

Contributor

Thanks for the fast review!

This is going to fail because of #47113
I'll update it in a bit

Contributor

sdroege commented Jan 3, 2018

Thanks for the fast review!

This is going to fail because of #47113
I'll update it in a bit

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 3, 2018

Member

Okay. @bors r-

Member

kennytm commented Jan 3, 2018

Okay. @bors r-

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 3, 2018

Contributor

For good measure, I'll also add unit tests for zipping the 3 iterators. It seems like this is not covered by any of the existing unit tests yet

Contributor

sdroege commented Jan 3, 2018

For good measure, I'll also add unit tests for zipping the 3 iterators. It seems like this is not covered by any of the existing unit tests yet

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 3, 2018

Contributor

This will now fail until #47113 is merged for real

Contributor

sdroege commented Jan 3, 2018

This will now fail until #47113 is merged for real

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

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

Contributor

bors commented Jan 3, 2018

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

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 3, 2018

Contributor

Rebased on top of latest master

Contributor

sdroege commented Jan 3, 2018

Rebased on top of latest master

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 3, 2018

Contributor

Good point, thanks. This should probably use checked_add and if it overflows then just use the length.

Will fix in a few hours, thanks for spotting this

Contributor

sdroege commented Jan 3, 2018

Good point, thanks. This should probably use checked_add and if it overflows then just use the length.

Will fix in a few hours, thanks for spotting this

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 4, 2018

Contributor

I fixed that now , but also see the discussion at #47126 (comment) and the following 3 comments. It might make sense to reconsider the whole TrustedRandomAccess iterator or at least the zip implementation based on it.

Actually this all seems to have been investigated back then already, so it should all be fine. See top of #47126 (comment)

Contributor

sdroege commented Jan 4, 2018

I fixed that now , but also see the discussion at #47126 (comment) and the following 3 comments. It might make sense to reconsider the whole TrustedRandomAccess iterator or at least the zip implementation based on it.

Actually this all seems to have been investigated back then already, so it should all be fine. See top of #47126 (comment)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 4, 2018

Contributor

I think we're good to go actually

Contributor

sdroege commented Jan 4, 2018

I think we're good to go actually

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 4, 2018

Contributor

Some benchmark results, based on the code from #47115 (comment) . Just running a 1920*1080*4 byte array through the normal chunks version.

Something like 1.5% difference (the new version is faster), but consistently. So even for things that llvm can't auto-vectorize it seems to be slightly faster.

Before

$ cargo +local bench
running 1 test
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

test tests::test_bgrx_to_grey ... bench:   5,858,100 ns/iter (+/- 160,940)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,842,103 ns/iter (+/- 143,723)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,858,885 ns/iter (+/- 193,095)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,853,836 ns/iter (+/- 184,774)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

After

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,764,354 ns/iter (+/- 237,465)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,764,800 ns/iter (+/- 298,007)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,780,834 ns/iter (+/- 234,143)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,783,144 ns/iter (+/- 242,745)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out
Contributor

sdroege commented Jan 4, 2018

Some benchmark results, based on the code from #47115 (comment) . Just running a 1920*1080*4 byte array through the normal chunks version.

Something like 1.5% difference (the new version is faster), but consistently. So even for things that llvm can't auto-vectorize it seems to be slightly faster.

Before

$ cargo +local bench
running 1 test
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

test tests::test_bgrx_to_grey ... bench:   5,858,100 ns/iter (+/- 160,940)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,842,103 ns/iter (+/- 143,723)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,858,885 ns/iter (+/- 193,095)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,853,836 ns/iter (+/- 184,774)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

After

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,764,354 ns/iter (+/- 237,465)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,764,800 ns/iter (+/- 298,007)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,780,834 ns/iter (+/- 234,143)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

$ cargo +local bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/deps/testing-611256b07a3291e9

running 1 test
test tests::test_bgrx_to_grey ... bench:   5,783,144 ns/iter (+/- 242,745)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out
@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 4, 2018

Member

Thanks again! Just wanna confirm, will the expression i * self.chunk_size overflow?

Member

kennytm commented Jan 4, 2018

Thanks again! Just wanna confirm, will the expression i * self.chunk_size overflow?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 4, 2018

Contributor

Thanks again! Just wanna confirm, will the expression i * self.chunk_size overflow?

Yes, but only if you call it with an i that is not in range for the slice. AFAIU this is fine, that's what the "unchecked" / unsafe part of the trait is about: you need to ensure beforehand that the item you want to access does actually exist.

Contributor

sdroege commented Jan 4, 2018

Thanks again! Just wanna confirm, will the expression i * self.chunk_size overflow?

Yes, but only if you call it with an i that is not in range for the slice. AFAIU this is fine, that's what the "unchecked" / unsafe part of the trait is about: you need to ensure beforehand that the item you want to access does actually exist.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Jan 4, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 4, 2018

Contributor

📌 Commit b69c124 has been approved by kennytm

Contributor

bors commented Jan 4, 2018

📌 Commit b69c124 has been approved by kennytm

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jan 4, 2018

Contributor

Thanks a lot @kennytm and @bluss :)

Contributor

sdroege commented Jan 4, 2018

Thanks a lot @kennytm and @bluss :)

kennytm added a commit to kennytm/rust that referenced this pull request Jan 4, 2018

Rollup merge of rust-lang#47142 - sdroege:trusted-random-access-chunk…
…s, r=kennytm

Implement TrustedRandomAccess for slice::{Chunks, ChunksMut, Windows}

As suggested by @bluss in rust-lang#47115 (comment)

bors added a commit that referenced this pull request Jan 5, 2018

Auto merge of #47195 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #46907, #47030, #47033, #47110, #47142, #47149, #47150, #47160, #47173, #47182
- Failed merges:
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

⌛️ Testing commit b69c124 with merge b98fd52...

Contributor

bors commented Jan 5, 2018

⌛️ Testing commit b69c124 with merge b98fd52...

bors added a commit that referenced this pull request Jan 5, 2018

Auto merge of #47142 - sdroege:trusted-random-access-chunks, r=kennytm
Implement TrustedRandomAccess for slice::{Chunks, ChunksMut, Windows}

As suggested by @bluss in #47115 (comment)
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: kennytm
Pushing b98fd52 to master...

Contributor

bors commented Jan 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: kennytm
Pushing b98fd52 to master...

@bors bors merged commit b69c124 into rust-lang:master Jan 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment