Skip to content

Constant new SmallVec<T, N> for arrays smaller than N #377

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

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

otcova
Copy link
Contributor

@otcova otcova commented Apr 27, 2025

What is this about

We should be able to create a constant SmallVec<T, N> of any size from 0 to N.
Currently, we have the following:

// Ok since N = 4
const DATA: SmallVec<i32, 4> = SmallVec::from_buf([1, 2, 3, 4]);

// Error
const DATA: SmallVec<i32, 4> = SmallVec::from_buf([1, 2, 3]);

This PR

I've implemented a new from_buf that takes an array of size [T; S] instead of [T; N].

Does it conflict with the current from_buf?

If we move it to a different function and keep the current from_buf how it is now, it's fine for me.

Performance concerns

I've copied the code into https://godbolt.org to check that the compiler is able to optimize the copy into a new buffer.
Here are the results:

use std::ptr::copy_nonoverlapping;
use std::mem::MaybeUninit;

#[inline]
pub const fn from_buf<T, const S: usize, const N: usize>(elements: [T; S]) -> MaybeUninit<[T; N]> {
    assert!(S <= N);
    let mut buf: MaybeUninit<[T; N]> = MaybeUninit::uninit();
    unsafe {
        copy_nonoverlapping(elements.as_ptr(), buf.as_mut_ptr() as *mut T, S);
    }
    std::mem::forget(elements);
    buf
}

#[inline(never)]
pub fn S_eq_N(num: [i32; 4]) -> MaybeUninit<[i32; 4]>  {
    from_buf(num)
}

#[inline(never)]
pub fn S_eq_N_ideal(num: [i32; 4]) -> MaybeUninit<[i32; 4]>  {
    MaybeUninit::new(num)
}

#[inline(never)]
pub fn S_less_than_N(num: [i32; 3]) -> MaybeUninit<[i32; 4]>  {
    from_buf(num)
}

#[inline(never)]
pub fn S_less_than_N_ideal(num: [i32; 3]) -> MaybeUninit<[i32; 4]>  {
    MaybeUninit::new([num[0], num[1], num[2], unsafe { MaybeUninit::uninit().assume_init() }])
}

Assembly with -C opt-level=1:

example::S_eq_N::h49a2fa36b6c32877:
        mov     rax, rdi
        movups  xmm0, xmmword ptr [rsi]
        movups  xmmword ptr [rdi], xmm0
        ret

example::S_eq_N_ideal::hfc5842e0def5e8e0:
        mov     rax, rdi
        movups  xmm0, xmmword ptr [rsi]
        movups  xmmword ptr [rdi], xmm0
        ret

example::S_less_than_N::hab2b36d48e2ebe64:
        mov     rax, rdi
        mov     ecx, dword ptr [rsi + 8]
        mov     dword ptr [rdi + 8], ecx
        mov     rcx, qword ptr [rsi]
        mov     qword ptr [rdi], rcx
        ret

example::S_less_than_N_ideal::he3468960a62a802e:
        mov     rax, rdi
        mov     rcx, qword ptr [rsi]
        mov     edx, dword ptr [rsi + 8]
        mov     qword ptr [rdi], rcx
        mov     dword ptr [rdi + 8], edx
        ret

Copy link
Collaborator

@mbrubeck mbrubeck left a comment

Choose a reason for hiding this comment

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

Thanks!

@mbrubeck mbrubeck added this pull request to the merge queue Apr 28, 2025
Merged via the queue into servo:v2 with commit 6ba116c Apr 28, 2025
2 of 5 checks passed
@mbrubeck
Copy link
Collaborator

This fails to build in Rust 1.57, so we will need to bump our MSRV if we want to ship this. It looks like it might require Rust 1.83?

(Also, I'm not sure why this made it through the merge queue without passing all of the tests. Looks like we need to fix our merge queue configuration.)

@mbrubeck mbrubeck mentioned this pull request Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants