Skip to content

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Apr 2, 2022

It was using Iterator::by_ref in the implementation, which ended up pessimizing it enough that, for example, it didn't vectorize when we tried it in the https://rust-lang.zulipchat.com/#narrow/stream/257879-project-portable-simd/topic/Reducing.20sum.20into.20wider.20types conversation.

Demonstration that the codegen test doesn't pass on the current nightly: https://rust.godbolt.org/z/Taxev5eMn

It was using `Iterator::by_ref` in the implementation, which ended up pessimizing it enough that, for example, it didn't vectorize when we tried it in the <https://rust-lang.zulipchat.com/#narrow/stream/257879-project-portable-simd/topic/Reducing.20sum.20into.20wider.20types> conversation.

Demonstration that the codegen test doesn't pass on the current nightly: <https://rust.godbolt.org/z/Taxev5eMn>
@rust-highfive
Copy link
Contributor

r? @joshtriplett

(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 Apr 2, 2022
@the8472
Copy link
Member

the8472 commented Apr 3, 2022

Huh, interesting. It previously used try_fold instead to not go through the impl<T: Iterator> Iterator for &mut T blanket impl (#87431), which was then removed in #89774 by adding #[inline] on the blanket impl which was sufficient for the vec::bench_flat_map_collect benchmark.
But yeah, this should be fine too.

@JohnCSimon JohnCSimon 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 Apr 24, 2022
@scottmcm
Copy link
Member Author

scottmcm commented May 9, 2022

Friendly one-month review reminder ping, @joshtriplett.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 13, 2022
@the8472
Copy link
Member

the8472 commented May 13, 2022

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented May 13, 2022

📌 Commit e8fc7ba has been approved by the8472

@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 May 13, 2022
@scottmcm scottmcm assigned the8472 and unassigned joshtriplett May 13, 2022
@bors
Copy link
Collaborator

bors commented May 14, 2022

⌛ Testing commit e8fc7ba with merge 9fbbe75...

@bors
Copy link
Collaborator

bors commented May 14, 2022

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 9fbbe75 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 14, 2022
@bors bors merged commit 9fbbe75 into rust-lang:master May 14, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 14, 2022
@bors bors mentioned this pull request May 14, 2022
@scottmcm scottmcm deleted the faster-array-intoiter-fold branch May 14, 2022 06:43
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9fbbe75): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

8 participants