Skip to content

Use TrustedRandomAccess for in-place iterators where possible#79846

Merged
bors merged 4 commits intorust-lang:masterfrom
the8472:inplace-tra
Mar 22, 2021
Merged

Use TrustedRandomAccess for in-place iterators where possible#79846
bors merged 4 commits intorust-lang:masterfrom
the8472:inplace-tra

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
Contributor

rust-highfive commented Dec 9, 2020

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

wesleywiser commented Dec 9, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Dec 9, 2020

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

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

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

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

@rust-timer
Copy link
Collaborator

rust-timer commented Dec 9, 2020

Queued 35e08efe204c0d413365fe2031938e476418f692 with parent c16d52d, future comparison URL.

@the8472

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

rust-timer commented Dec 10, 2020

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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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 added 3 commits March 21, 2021 20:54
@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
Collaborator

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
Collaborator

bors commented Mar 22, 2021

⌛ Testing commit 2d8be45 with merge 3538577...

@bors
Copy link
Collaborator

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

1 more reviewer

@pickfire pickfire pickfire left review comments

Reviewers whose approvals may not affect merge requirements

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.