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

Jump to bottom

Use TrustedRandomAccess for in-place iterators where possible #79846

Merged
merged 4 commits into from Mar 22, 2021
Merged

Use TrustedRandomAccess for in-place iterators where possible #79846

merged 4 commits into from Mar 22, 2021

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Dec 9, 2020

This can speed up in-place iterators containing simple casts and transmutes from Copy types to any type of same size. !Copy types can't be optimized since TrustedRandomAccess isn't implemented for those iterators.

 name                  on.b ns/iter     o1.b ns/iter     diff ns/iter   diff %  speedup
 vec::bench_transmute  20 (40000 MB/s)  12 (66666 MB/s)            -8  -40.00%   x 1.67

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(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 Dec 9, 2020
@camelid
Copy link
Member

camelid commented Dec 9, 2020 

---- [codegen] codegen/vec-in-place.rs stdout ----

error: verification with 'FileCheck' failed
status: exit code: 1
command: "/usr/lib/llvm-9/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/vec-in-place/vec-in-place.ll" "/checkout/src/test/codegen/vec-in-place.rs"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
/checkout/src/test/codegen/vec-in-place.rs:10:16: error: CHECK-NOT: excluded string found in input
 // CHECK-NOT: call
               ^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/vec-in-place/vec-in-place.ll:167:2: note: found here
 call void @llvm.lifetime.start.p0i8(i64 32, i8* nonnull %2), !noalias !20
 ^~~~

------------------------------------------

@camelid camelid added A-iterators Area: Iterators I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 9, 2020
@the8472
Copy link
Member Author

the8472 commented Dec 9, 2020

I assume that was due to the older LLVM version, I added a min requirement to the codegen test since this is just an optimization.

@wesleywiser
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 9, 2020

⌛ Trying commit 04cda95ab5f7ebb4d734ff3e94124159e7f1ab61 with merge 35e08efe204c0d413365fe2031938e476418f692...

@bors
Copy link
Contributor

bors commented Dec 9, 2020

☀️ Try build successful - checks-actions
Build commit: 35e08efe204c0d413365fe2031938e476418f692 (35e08efe204c0d413365fe2031938e476418f692)

@rust-timer
Copy link
Collaborator

Queued 35e08efe204c0d413365fe2031938e476418f692 with parent c16d52d, future comparison URL.

@the8472

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (35e08efe204c0d413365fe2031938e476418f692): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@the8472
Copy link
Member Author

the8472 commented Dec 10, 2020

syn-opt is noisy so we can probably ignore that, other than that a few tiny improvements.

@bors
Copy link
Contributor

bors commented Dec 31, 2020

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

@the8472 the8472 force-pushed the inplace-tra branch 2 times, most recently from 8e23a6d to 783fa23 Compare December 31, 2020 01:26
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 force-pushed the inplace-tra branch 2 times, most recently from e3d7764 to ad2fe3e Compare December 31, 2020 02:17
@crlf0710 crlf0710 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 Jan 15, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 22, 2021

r? @m-ou-se

@crlf0710 crlf0710 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 Mar 5, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2021

📌 Commit 5c9c5bd5807bef81f9989728d00a6985f4375e55 has been approved by m-ou-se

@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 Mar 21, 2021
@bors
Copy link
Contributor

bors commented Mar 21, 2021

⌛ Testing commit 5c9c5bd5807bef81f9989728d00a6985f4375e55 with merge 8435e466f5bdc06e9cf3a01f84be762629981f8b...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 21, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 21, 2021
@the8472
add transmute-via-iterators bench
a1a04e0
@the8472
specialize in-place collection further via TrustedRandomAccess
6c67e55 
This allows the optimizer to turn certain iterator pipelines such as

```rust
let vec = vec![0usize; 100];
vec.into_iter().map(|e| e as isize).collect::<Vec<_>>()
```

into a noop.

The optimization only applies when iterator sources are  `T: Copy`
since `impl TrustedRandomAccess for IntoIter<T>`.
No such requirement applies to the output type (`Iterator::Item`).
@the8472
add codegen test
17f4c2a
@m-ou-se m-ou-se 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2021
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2021

📌 Commit 2d8be45 has been approved by m-ou-se

@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 Mar 21, 2021
@bors
Copy link
Contributor

bors commented Mar 22, 2021

⌛ Testing commit 2d8be45 with merge 3538577...

@bors
Copy link
Contributor

bors commented Mar 22, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 3538577 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2021
@bors bors merged commit 3538577 into rust-lang:master Mar 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pickfire pickfire pickfire left review comments

Assignees

@m-ou-se m-ou-se

Labels
A-iterators Area: Iterators I-slow Issue: Problems and improvements with respect to performance of generated code. merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.

None yet