Optimize ArrayVec::clone() to behave like Copy when T: Copy#315
Conversation
484c9d7 to
7c25709
Compare
|
Sounds useful. Duplicating the extend code is not my favourite solution. I can't reproduce your performance gains right now on this development machine (my arch is an intel tiger lake laptop) so they are probably arch dependent. Please evaluate this improvement as an alternatvie, which might be much simpler for a similar gain, hopefully? I have looked at generated code, and I can see that it's a middle ground, not an equivalent improvement. diff --git src/arrayvec.rs src/arrayvec.rs
index eec77328..b590cd9d 100644
--- src/arrayvec.rs
+++ src/arrayvec.rs
@@ -1193,7 +1193,12 @@ impl<T, const CAP: usize> Clone for ArrayVec<T, CAP>
where T: Clone
{
fn clone(&self) -> Self {
- self.iter().cloned().collect()
+ let mut new = ArrayVec::<T, CAP>::new();
+ // Safety: we know it fits the capacity
+ unsafe {
+ new.extend_from_iter::<_, false>(self.iter().cloned());
+ }
+ new
}
fn clone_from(&mut self, rhs: &Self) { |
|
Nice suggestion, it eliminate the checks and the panic paths. running 5 tests
test clone_16 ... bench: 4 ns/iter (+/- 2) = 4000 MB/s
test clone_32 ... bench: 10 ns/iter (+/- 1) = 3200 MB/s
test clone_512 ... bench: 201 ns/iter (+/- 38) = 2547 MB/s
test clone_64 ... bench: 24 ns/iter (+/- 3) = 2666 MB/s
test clone_8 ... bench: 2 ns/iter (+/- 0) = 4000 MB/sSlightly slower for the smaller array. The measurement are very short, but the diff is consistent between runs. This is the asm: hello::do_work:
Lfunc_begin6:
.cfi_startproc
sub sp, sp, #48
.cfi_def_cfa_offset 48
stp x20, x19, [sp, #16]
stp x29, x30, [sp, #32]
add x29, sp, #32
.cfi_def_cfa w29, 16
.cfi_offset w30, -8
.cfi_offset w29, -16
.cfi_offset w19, -24
.cfi_offset w20, -32
mov x19, x0
ldr w20, [x1]
cbz w20, LBB6_2
add x0, sp, #8
add x1, x1, #4
mov x2, x20
bl _memcpy
ldr x8, [sp, #8]
b LBB6_3
LBB6_2:
LBB6_3:
str w20, [x19]
stur x8, [x19, #4]
.cfi_def_cfa wsp, 48
ldp x29, x30, [sp, #32]
ldp x20, x19, [sp, #16]
add sp, sp, #48
.cfi_def_cfa_offset 0
.cfi_restore w30
.cfi_restore w29
.cfi_restore w19
.cfi_restore w20
retAnother solution that produce a very similar assembly and comparable measurements is adding Its hard for me to give up on the branchless, Copy-like asm. Do you think there is a way to edit it to be more reasonable and accepted? The main requirement is to copy the uninit data from len to CAP, so the compiler can just ignore len for Copy types. |
|
I use godbolt to see the asm created on x64, huge difference This PRhttps://godbolt.org/z/eW3q54cxx example::do_work::ha9fd69377e196564:
mov rax, rdi
movzx ecx, word ptr [rsi]
mov word ptr [rdi], cx
mov rcx, qword ptr [rsi + 2]
mov qword ptr [rdi + 2], rcx
retCHECK=falsehttps://godbolt.org/z/8T6qoh6nq example::do_work::ha9fd69377e196564:
push r14
push rbx
push rax
mov rbx, rdi
movzx r14d, word ptr [rsi]
test r14, r14
je .LBB1_1
add rsi, 2
mov rdi, rsp
mov rdx, r14
call qword ptr [rip + memcpy@GOTPCREL] # <-- call to memcpy
mov rax, qword ptr [rsp]
jmp .LBB1_3
.LBB1_1:
.LBB1_3:
mov word ptr [rbx], r14w
mov qword ptr [rbx + 2], rax
mov rax, rbx
add rsp, 8
pop rbx
pop r14
retOriginal codehttps://godbolt.org/z/5h7cqneeE example::do_work::ha9fd69377e196564:
push rbx
mov rax, rdi
movzx ebx, word ptr [rsi]
test rbx, rbx
je .LBB1_1
add rbx, 2
movzx ecx, byte ptr [rsi + 2]
cmp ebx, 3
jne .LBB1_5
mov si, 1
jmp .LBB1_19
.LBB1_1:
xor esi, esi
jmp .LBB1_19
.LBB1_5:
movzx edx, byte ptr [rsi + 3]
cmp ebx, 4
jne .LBB1_7
mov si, 2
jmp .LBB1_19
.LBB1_7:
movzx edi, byte ptr [rsi + 4]
cmp ebx, 5
jne .LBB1_9
mov si, 3
jmp .LBB1_19
.LBB1_9:
movzx r8d, byte ptr [rsi + 5]
cmp ebx, 6
jne .LBB1_11
mov si, 4
jmp .LBB1_19
.LBB1_11:
movzx r9d, byte ptr [rsi + 6]
cmp ebx, 7
jne .LBB1_13
mov si, 5
jmp .LBB1_19
.LBB1_13:
movzx r10d, byte ptr [rsi + 7]
cmp ebx, 8
jne .LBB1_15
mov si, 6
jmp .LBB1_19
.LBB1_15:
movzx r11d, byte ptr [rsi + 8]
cmp ebx, 9
jne .LBB1_17
mov si, 7
jmp .LBB1_19
.LBB1_17:
cmp ebx, 10
jne .LBB1_4
movzx ebx, byte ptr [rsi + 9]
mov si, 8
.LBB1_19:
mov word ptr [rax], si
mov byte ptr [rax + 2], cl
mov byte ptr [rax + 3], dl
mov byte ptr [rax + 4], dil
mov byte ptr [rax + 5], r8b
mov byte ptr [rax + 6], r9b
mov byte ptr [rax + 7], r10b
mov byte ptr [rax + 8], r11b
mov byte ptr [rax + 9], bl
pop rbx
ret
.LBB1_4:
lea rdi, [rip + .Lanon.466b45355eb502565aec0b178a18bdf9.1]
call qword ptr [rip + example::extend_panic::ha5d83a66ac79d21b@GOTPCREL] |
|
I maintainer-pushed an addition of commits which I think makes the changes reasonable/palatable. Part of the solution is cleaning up extend so that it's not so sad to look at. There is a risk of losing extend optimizations - because the old benchmarks were not really functional (they were testing something that was optimized out - and some of them still may be). I simplified the clone implementation, removed everything possible while preserving the "simple struct copy" as a benchmark. From you, but here it is in local form, using extern crate arrayvec;
use arrayvec::ArrayVec;
#[inline(never)]
pub fn do_work(array: &ArrayVec<u8, 8>) -> ArrayVec<u8, 8> {
array.clone()
}rustc --edition 2021 -C opt-level=3 --crate-type lib testfun.rs -L target/release/deps --emit=asm -o -Output: ZN7testfun7do_work17h27b711b5dad6bbccE:
.cfi_startproc
movq %rdi, %rax
movl (%rsi), %ecx
movl %ecx, (%rdi)
movq 4(%rsi), %rcx
movq %rcx, 4(%rdi)
retq |
|
Let me know if this simplification of the clone optimization still preserves performance in your tests/your eyes. |
|
|
||
| // Safety: copy of MaybeUninit to MaybeUninit | ||
| unsafe { | ||
| ptr::copy_nonoverlapping(self.xs[i].as_ptr(), array.xs[i].as_mut_ptr(), 1) |
There was a problem hiding this comment.
| ptr::copy_nonoverlapping(self.xs[i].as_ptr(), array.xs[i].as_mut_ptr(), 1) | |
| ptr::copy_nonoverlapping(&self.xs[i], &mut array.xs[i], 1) |
Not sure about the exact semantics of UB, but instead of copy_nonoverlapping::<T>(..) we should probably use copy_nonoverlapping::<MaybeUninit<T>>(..)
There was a problem hiding this comment.
Yes, thank you, I also was reviewing this and agree.
|
both code and asm looks great, thanks! wrote a small comment, want me to fix it? wdyt about #314? |
|
I fixed that mistake, thanks. I also back-edited one of your changes so that the bench uses The clone code is now in fact even simpler (and it could be simpler, literally |
|
I'm sorry for the back and forth, but now that clone and extend are untangled, the extend changes will be in PR #316, which should feel less weird for the contributor who got stuck with extra changes in their PR. |
Desired feature is that clone of ArrayVec<u8, 8> should look like a simple struct copy (regardless of how full the arrayvec is). Simplify the clone implementation while preserving this. Scope guard for length not found to be necessary. Write element and length possible but it is equivalent to push_unchecked which also worked here.
I reverted this (back to bencher::black_box). Mainly because it changes the benchmark so much that I don't see the difference; but I'm not sure I see the difference in the benchmark with the revert either - safer to leave it as you wrote it, feel free to revisit that or not (codegen test was clearly easier to use as goal here). Thanks for a nice optimization |
First of all, love the library! use it all the time 😄
This PR optimize
ArrayVec::clone()by copying elements pastlenup toCAP. This sounds weird and counter intuitive, but for small arrays of trivial types this optimizations remove bound checks and compiles to very simple assembly.It also include a small benchmark for
ArrayVec::clone()that demonstrate the speed up.As you can see in the code, i bounded the optimization to 128 bytes, but apparently even 512 CAP arrays get a speed up. I didnt check, but my guess is that the asm is simpler without the iterator.
For a simple function like this one:
This is the assembly on my machine after the PR:
This is the same function before the PR:
Thanks for the support in advance ❤️