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

optimize zipping over array iterators #115515

Merged
merged 1 commit into from
Oct 15, 2023
Merged

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Sep 3, 2023

Fixes #115339 (somewhat)

the new assembly:

zip_arrays:
        .cfi_startproc
        vmovups (%rdx), %ymm0
        leaq    32(%rsi), %rcx
        vxorps  %xmm1, %xmm1, %xmm1
        vmovups %xmm1, -24(%rsp)
        movq    $0, -8(%rsp)
        movq    %rsi, -88(%rsp)
        movq    %rdi, %rax
        movq    %rcx, -80(%rsp)
        vmovups %ymm0, -72(%rsp)
        movq    $0, -40(%rsp)
        movq    $32, -32(%rsp)
        movq    -24(%rsp), %rcx
        vmovups (%rsi,%rcx), %ymm0
        vorps   -72(%rsp,%rcx), %ymm0, %ymm0
        vmovups %ymm0, (%rsi,%rcx)
        vmovups (%rsi), %ymm0
        vmovups %ymm0, (%rdi)
        vzeroupper
        retq

This is still longer than the slice version given in the issue but at least it eliminates the terrible vpextrb/orb chain. I guess this is due to excessive memcpys again (haven't looked at the llvmir)?

The TrustedLen specialization is a drive-by change since I had to do something for the default impl anyway to be able to specialize the TrustedRandomAccessNoCoerce impl.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 3, 2023
@the8472
Copy link
Member Author

the8472 commented Sep 3, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 3, 2023
@bors
Copy link
Contributor

bors commented Sep 3, 2023

⌛ Trying commit 0580b27 with merge 3effcd54b6ab31955a45d85d1ca90746310d294b...

@bors
Copy link
Contributor

bors commented Sep 3, 2023

☀️ Try build successful - checks-actions
Build commit: 3effcd54b6ab31955a45d85d1ca90746310d294b (3effcd54b6ab31955a45d85d1ca90746310d294b)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3effcd54b6ab31955a45d85d1ca90746310d294b): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.7%, -0.6%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.5% [-0.7%, 0.7%] 7

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.7% [-6.6%, -2.8%] 2
Improvements ✅
(secondary)
-3.5% [-4.7%, -1.4%] 4
All ❌✅ (primary) -2.2% [-6.6%, 2.7%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.1% [1.1%, 1.1%] 1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 0.5%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.0% [-0.1%, -0.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [-0.1%, 0.5%] 10

Bootstrap: 629.273s -> 628.851s (-0.07%)
Artifact size: 316.72 MiB -> 316.72 MiB (0.00%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 4, 2023
#[inline]
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
// SAFETY: The caller must provide an idx that is in bound of the remainder.
unsafe { self.data.as_ptr().add(self.alive.start()).add(idx).cast::<T>().read() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, of course, random access to an array iterator makes a ton of sense 👍

@scottmcm
Copy link
Member

scottmcm commented Oct 6, 2023

Hmm, the LLVM IR for the array version in nightly has load <32 x i8>, which makes me wonder whether #111999 is involved here somehow.

Why that would make the codegen so much worse is unclear to me, though. Does the same thing happen for different lengths? Or is it just 32 that goes bad? That happens to be exactly the somewhat-arbitrary threshold in that PR.

Regardless, I think doing this to enable random access for array::IntoIter seems like a good idea, so
@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2023

📌 Commit 0580b27 has been approved by scottmcm

It is now in the queue for this repository.

@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 Oct 6, 2023
@bors
Copy link
Contributor

bors commented Oct 6, 2023

⌛ Testing commit 0580b27 with merge d603d0f...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 6, 2023
optimize zipping over array iterators

Fixes rust-lang#115339 (somewhat)

the new assembly:

```asm
zip_arrays:
        .cfi_startproc
        vmovups (%rdx), %ymm0
        leaq    32(%rsi), %rcx
        vxorps  %xmm1, %xmm1, %xmm1
        vmovups %xmm1, -24(%rsp)
        movq    $0, -8(%rsp)
        movq    %rsi, -88(%rsp)
        movq    %rdi, %rax
        movq    %rcx, -80(%rsp)
        vmovups %ymm0, -72(%rsp)
        movq    $0, -40(%rsp)
        movq    $32, -32(%rsp)
        movq    -24(%rsp), %rcx
        vmovups (%rsi,%rcx), %ymm0
        vorps   -72(%rsp,%rcx), %ymm0, %ymm0
        vmovups %ymm0, (%rsi,%rcx)
        vmovups (%rsi), %ymm0
        vmovups %ymm0, (%rdi)
        vzeroupper
        retq
```

This is still longer than the slice version given in the issue but at least it eliminates the terrible  `vpextrb`/`orb` chain. I guess this is due to excessive memcpys again (haven't looked at the llvmir)?

The `TrustedLen` specialization is a drive-by change since I had to do something for the default impl anyway to be able to specialize the `TrustedRandomAccessNoCoerce` impl.
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 6, 2023

💔 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 Oct 6, 2023
@scottmcm
Copy link
Member

scottmcm commented Oct 6, 2023

@bors r-

Hmm, I wonder if the upstream changes mentioned in #115339 (comment) will obviate this at all...

@bors bors 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 Oct 6, 2023
@the8472
Copy link
Member Author

the8472 commented Oct 6, 2023

Hmm, I wonder if the upstream changes mentioned in #115339 (comment) will obviate this at all...

I don't know but guess that random access could still be beneficial for more complex combinations of array iterators + zip.

@the8472
Copy link
Member Author

the8472 commented Oct 6, 2023

@rustbot ready

I think the issue was that nopt builds disable optimizations for tests too if an optimization level hasn't been set explicitly

@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 Oct 6, 2023
@scottmcm
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Oct 14, 2023

📌 Commit b018ad3 has been approved by scottmcm

It is now in the queue for this repository.

@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 Oct 14, 2023
@bors
Copy link
Contributor

bors commented Oct 15, 2023

⌛ Testing commit b018ad3 with merge 0d410be...

@bors
Copy link
Contributor

bors commented Oct 15, 2023

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 0d410be to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 15, 2023
@bors bors merged commit 0d410be into rust-lang:master Oct 15, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 15, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0d410be): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.3% [-0.4%, -0.2%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.3% [-0.4%, -0.2%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.7% [1.1%, 8.5%] 4
Regressions ❌
(secondary)
4.5% [4.5%, 4.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-4.0%, -1.6%] 3
All ❌✅ (primary) 3.7% [1.1%, 8.5%] 4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 0.2%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.2%, -0.1%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [-0.2%, 0.2%] 10

Bootstrap: 624.559s -> 624.729s (0.03%)
Artifact size: 305.55 MiB -> 305.51 MiB (-0.01%)

@rustbot rustbot removed the perf-regression Performance regression. label Oct 15, 2023
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <bdgdlm@outlook.com>
Co-authored-by: Colin Finck <colin@reactos.org>
Co-authored-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com>
Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>
Co-authored-by: Trevor Gross <tmgross@umich.edu>
Co-authored-by: Evan Merlock <evan@merlock.dev>
Co-authored-by: joboet <jonasboettiger@icloud.com>
Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
Co-authored-by: Mark Rousskov <mark.simulacrum@gmail.com>
Co-authored-by: onur-ozkan <work@onurozkan.dev>
Co-authored-by: Nicholas Nethercote <n.nethercote@gmail.com>
Co-authored-by: The 8472 <git@infinite-source.de>
Co-authored-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Co-authored-by: reez12g <reez12g@gmail.com>
Co-authored-by: Jakub Beránek <berykubik@gmail.com>
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 29, 2024
Remove my `scalar_copy_backend_type` optimization attempt

I added this back in rust-lang#111999 , but I no longer think it's a good idea
- It had to get scaled back to only power-of-two things to not break a bunch of targets
- LLVM seems to be getting better at memcpy removal anyway
- Introducing vector instructions has seemed to sometimes (rust-lang#115515 (comment)) make autovectorization worse

So this removes it from the codegen crates entirely, and instead just tries to use <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/builder/trait.BuilderMethods.html#method.typed_place_copy> instead of direct `memcpy` so things will still use load/store when a type isn't `OperandValue::Ref`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
…-errors

Remove my `scalar_copy_backend_type` optimization attempt

I added this back in rust-lang#111999 , but I no longer think it's a good idea
- It had to get scaled back to only power-of-two things to not break a bunch of targets
- LLVM seems to be getting better at memcpy removal anyway
- Introducing vector instructions has seemed to sometimes (rust-lang#115515 (comment)) make autovectorization worse

So this removes it from the codegen crates entirely, and instead just tries to use <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/builder/trait.BuilderMethods.html#method.typed_place_copy> instead of direct `memcpy` so things will still use load/store when a type isn't `OperandValue::Ref`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
…-errors

Remove my `scalar_copy_backend_type` optimization attempt

I added this back in rust-lang#111999 , but I no longer think it's a good idea
- It had to get scaled back to only power-of-two things to not break a bunch of targets
- LLVM seems to be getting better at memcpy removal anyway
- Introducing vector instructions has seemed to sometimes (rust-lang#115515 (comment)) make autovectorization worse

So this removes it from the codegen crates entirely, and instead just tries to use <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/builder/trait.BuilderMethods.html#method.typed_place_copy> instead of direct `memcpy` so things will still use load/store when a type isn't `OperandValue::Ref`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
…-errors

Remove my `scalar_copy_backend_type` optimization attempt

I added this back in rust-lang#111999 , but I no longer think it's a good idea
- It had to get scaled back to only power-of-two things to not break a bunch of targets
- LLVM seems to be getting better at memcpy removal anyway
- Introducing vector instructions has seemed to sometimes (rust-lang#115515 (comment)) make autovectorization worse

So this removes it from the codegen crates entirely, and instead just tries to use <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/builder/trait.BuilderMethods.html#method.typed_place_copy> instead of direct `memcpy` so things will still use load/store when a type isn't `OperandValue::Ref`.
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.

Performance regression of array::IntoIter vs slice::Iter
6 participants