Default for arrays via const generics #74254

MikailBag · 2020-07-11T19:35:47Z

Sorry, I created PR too early :)
It is WIP currently.
UPD: no longer WIP, but blocked on codegen issues.

rust-highfive · 2020-07-11T19:35:50Z

(rust_highfive has picked a reviewer for you, use r? to override)

MikailBag · 2020-07-11T22:14:46Z

Well, I think this PR works and can be reviewed.
Two tests are failing, but I don't know why. Their error message does not mention 'Default' at all.

dtolnay

I haven't reviewed closely yet, but the generated code looks quite a bit worse than before so I would be concerned about the performance and code size implications of this.

https://rust.godbolt.org/z/3dxGE5

extern crate core;
use core::mem::{self, MaybeUninit};
use core::ptr;

type T = String;
const N: usize = 2;

pub fn before() -> [T; N] {
    Default::default()
}

pub fn after() -> [T; N] {
    struct Wrapper {
        data: MaybeUninit<[T; N]>,
        init: usize,
    }

    impl Drop for Wrapper {
        #[inline]
        fn drop(&mut self) {
            debug_assert!(self.init <= N);

            let ptr = self.data.as_mut_ptr() as *mut T;
            let initialized_part = ptr::slice_from_raw_parts_mut(ptr, self.init);
            unsafe {
                ptr::drop_in_place(initialized_part);
            }
        }
    }

    let mut w = Wrapper { data: MaybeUninit::uninit(), init: 0 };
    let array_pointer = w.data.as_mut_ptr() as *mut T;
    for i in 0..N {
        assert!(N <= isize::MAX as usize);
        unsafe {
            let elem_ptr = array_pointer.add(i);
            elem_ptr.write(T::default());
        }
        w.init += 1;
    }

    let data = unsafe { w.data.as_ptr().read() };
    mem::forget(w);
    data
}

example::before:
        mov     rax, rdi
        mov     qword ptr [rdi], 1
        vxorps  xmm0, xmm0, xmm0
        vmovups xmmword ptr [rdi + 8], xmm0
        mov     qword ptr [rdi + 24], 1
        vmovups xmmword ptr [rdi + 32], xmm0
        ret

example::after:
        sub     rsp, 56
        mov     qword ptr [rsp], 1
        vxorps  xmm0, xmm0, xmm0
        vmovups xmmword ptr [rsp + 8], xmm0
        mov     qword ptr [rsp + 24], 1
        vmovups xmmword ptr [rsp + 32], xmm0
        mov     rax, rdi
        mov     qword ptr [rsp + 48], 2
        mov     rcx, qword ptr [rsp]
        mov     qword ptr [rdi], rcx
        vmovups xmm0, xmmword ptr [rsp + 8]
        vmovups xmmword ptr [rdi + 8], xmm0
        mov     rcx, qword ptr [rsp + 24]
        mov     qword ptr [rdi + 24], rcx
        mov     rcx, qword ptr [rsp + 16]
        mov     qword ptr [rdi + 16], rcx
        mov     rcx, qword ptr [rsp + 24]
        mov     qword ptr [rdi + 24], rcx
        vmovups xmm0, xmmword ptr [rsp + 32]
        vmovups xmmword ptr [rdi + 32], xmm0
        add     rsp, 56
        ret

dtolnay · 2020-07-11T22:40:24Z

@bors try

bors · 2020-07-11T22:40:41Z

⌛ Trying commit 3d3a7bd02207ae5d2be3bd3ac776b4b0ea15e1e1 with merge 8205f8afb8c63490368efc0d9db81e4e2b5830d8...

src/libcore/array/mod.rs

bors · 2020-07-12T01:35:31Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 8205f8afb8c63490368efc0d9db81e4e2b5830d8 (8205f8afb8c63490368efc0d9db81e4e2b5830d8)

lcnr · 2020-07-12T09:34:17Z

The test failures seems like a problem with winnowing. The bound in default_array has two candidates now

[T, 0] and [T; N]: NonZero, we then winnow this down to just [T; 0] which does not implement Foo.
This seems fairly interesting... will try this out locally

kpp · 2020-07-12T09:39:05Z

src/libcore/array/mod.rs

+// this is Default implementation for zero-sized arrays
+#[stable(since = "1.4.0", feature = "array_default")]
+impl<T> Default for [T; 0] {
+    #[inline(always)]


The previous version did not have inline attr

lcnr · 2020-07-12T10:11:34Z

Minimized:

fn default_array<T, const N: usize>() -> [T; N]
where
    [T; N]: Default
{
    Default::default()
}

fn main() {
    let _: [u8; 4] = default_array();
}

shepmaster · 2020-07-12T12:09:38Z

src/libcore/array/mod.rs

+        struct Guard<T, const N: usize> {
+            data: *mut T,
+            init: usize,
+        }


This is effectively implementing FromIterator for arrays. I think extracting this making it reusable across other places would make sense.

Well, I imagine something like:

// in core::mem pub(crate) struct PartialBuffer<T, const N> { buf: [MaybeUninit<T>; N], init: usize } impl PartialBuffer<_, _> { pub(crate) fn new() -> Self {..} pub(crate) unsafe fn push(&mut self, value: T) {..} // or maybe pub(crate) unsafe fn next_place(&mut self) -> &mut MaybeUninit<T> {..} to reduce copying pub(crate) unsafe fn assume_init(self) -> [T; N] {..} pub(crate) fn get_ref(&self) -> &[T] {..} pub(crate) fn get_mut(&mut self) -> &mut [T] {..} }

shepmaster · 2020-07-12T12:12:29Z

src/libcore/array/mod.rs

+    #[inline]
+    fn default() -> [T; N] {
+        use crate::mem::MaybeUninit;
+        let mut data: MaybeUninit<[T; N]> = MaybeUninit::uninit();


Why not make the guard type itself hold the MaybeUninit?

Previous version did so.
I tried keeping MaybeUninit outside of Guard to see whether it simplifies code and whether it optimizes generated ASM.

dtolnay

Newest version of the code, in godbolt: https://rust.godbolt.org/z/Pc6175.

example::before:
        mov     rax, rdi
        mov     qword ptr [rdi], 1
        vxorps  xmm0, xmm0, xmm0
        vmovups xmmword ptr [rdi + 8], xmm0
        mov     qword ptr [rdi + 24], 1
        vmovups xmmword ptr [rdi + 32], xmm0
        ret

example::after:
        sub     rsp, 48
        mov     rax, rdi
        mov     qword ptr [rsp], 1
        vxorps  xmm0, xmm0, xmm0
        vmovups xmmword ptr [rsp + 8], xmm0
        mov     qword ptr [rsp + 24], 1
        vmovups xmmword ptr [rsp + 32], xmm0
        mov     rcx, qword ptr [rsp]
        mov     qword ptr [rdi], rcx
        vmovups xmm0, xmmword ptr [rsp + 8]
        vmovups xmmword ptr [rdi + 8], xmm0
        mov     rcx, qword ptr [rsp + 24]
        mov     qword ptr [rdi + 24], rcx
        mov     rcx, qword ptr [rsp + 16]
        mov     qword ptr [rdi + 16], rcx
        mov     rcx, qword ptr [rsp + 24]
        mov     qword ptr [rdi + 24], rcx
        vmovups xmm0, xmmword ptr [rsp + 32]
        vmovups xmmword ptr [rdi + 32], xmm0
        add     rsp, 48
        ret

It appears to me that the difference is entirely a consequence of "return value optimization" not kicking in for the MaybeUninit for some reason, which seems fixable in the compiler. Changing the following two lines fixes the performance issue and gets exactly the same machine code as before (but isn't correct).

- let mut data: MaybeUninit<[T; N]> = MaybeUninit::uninit();
+ let mut data: [T; N] = unsafe { mem::uninitialized() }; // unsound

- unsafe { data.assume_init() }
+ data

example::before:
        mov     rax, rdi
        mov     qword ptr [rdi], 1
        vxorps  xmm0, xmm0, xmm0
        vmovups xmmword ptr [rdi + 8], xmm0
        mov     qword ptr [rdi + 24], 1
        vmovups xmmword ptr [rdi + 32], xmm0
        ret

example::after:
        mov     rax, rdi
        mov     qword ptr [rdi], 1
        vxorps  xmm0, xmm0, xmm0
        vmovups xmmword ptr [rdi + 8], xmm0
        mov     qword ptr [rdi + 24], 1
        vmovups xmmword ptr [rdi + 32], xmm0
        ret

lcnr · 2020-07-12T12:59:50Z

@MikailBag thanks for this PR, we ended up talking about this implementation and the winnowing problem on zulip: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/good.20example.20for.20const.20generics/near/203642562
and I ended up with a solution which ended up evading that problem entirely about which I personally feel a lot more comfortable.

Do you want to try and incorporate that impl in your PR or should I open a new one for it?

/// A trait implemented by all arrays which are either empty or contain a type implementing `Default`.
#[unstable(feature = "array_default_internals", reason = "implementation detail", issue = "none")]
#[marker]
pub trait ArrayDefault {}

#[unstable(feature = "array_default_internals", reason = "implementation detail", issue = "none")]
impl<T> ArrayDefault for [T; 0] {}

#[unstable(feature = "array_default_internals", reason = "implementation detail", issue = "none")]
impl<T: Default, const N: usize> ArrayDefault for [T; N] {}

trait DefaultHack {
    fn default_hack() -> Self;
}

impl<T> DefaultHack for T {
    default fn default_hack() -> Self {
        unreachable!();
    }
}

impl<T: Default> DefaultHack for T {
    fn default_hack() -> Self {
        Default::default()
    }
}

#[stable(since = "1.4.0", feature = "array_default")]
impl<T, const N: usize> Default for [T; N]
where
    [T; N]: ArrayDefault
{
    // ...
}

MikailBag · 2020-07-12T13:02:55Z

@dtolnay thanks for your wonderful investigation!
I know pretty little about LLVM, codegen and assembly, so I don't think I can solve 74267 mysef.
I see following ways forward:

Wait until MaybeUninit performance problems are solved.
Ignore performance regression.
Combine macros with const generics: for N <= 32 generate impls via macros, and use const generics for all other lengthes.
That way, we have no performance regressions.

dtolnay · 2020-07-12T13:05:20Z

3 seems best if possible.

shepmaster · 2020-07-12T13:43:23Z

3. Combine macros with const generics: for N <= 32 generate impls via macros, and use const generics for all other lengthes.

If we are going to add back the LengthAtMost32 bound, then the const generic version will never be used 🙂

MikailBag · 2020-07-12T14:41:53Z

@dtolnay I implemented third approach, tests pass (wow 😄 ).
@shepmaster I marked const-generics-based impl as unstable. Should it work?

lcnr · 2020-07-12T14:53:23Z

I do not feel comfortable with the current approach.

this instantly stabilizes the const generic Default impls without giving them the imo required test coverage as most of the existing code was written with the 32 elements limitation, therefore not using the new impl.
it uses negative impls to implement (something even stronger than) specialization, which we aren't doing anywhere else afaik. This previously resulted in Default for arrays via const generics #74254 (comment) failing and in general results in strange type inference issues. The current implementation fails for:

#![feature(const_generics)]

fn break_me<T, const N: usize>() -> [T; N]
where
    [T; N]: Default 
{
    Default::default()
}
fn main() {
    let _: [u8; 35] = break_me();
}

I personally would like to try and perf what happens if we just use const generics for all impls and
use the approach outlined in #74254 (comment)

@shepmaster I marked const-generics-based impl as unstable. Should it work?

While these impls are bound on an unstable trait, they are still instantly stable.

lcnr · 2020-07-12T16:05:40Z

The current impl also fails for

#![feature(const_generics)]

struct Foo<const N: usize>([u8; N]);

impl<const N: usize> Foo<N> {
    fn new() -> Self {
        Foo(Default::default())
    }
}

fn main() {}

with

error[E0277]: the trait bound `[u8; N]: std::default::Default` is not satisfied
  --> /home/lcnr/rust7/src/test/ui/__check/fk.rs:7:13
   |
LL |         Foo(Default::default())
   |             ^^^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `[u8; N]`
   |
   = help: the following implementations were found:
             <&[T] as std::default::Default>
             <&mut [T] as std::default::Default>
             <[T; 0] as std::default::Default>
             <[T; 10] as std::default::Default>
           and 32 others
   = note: required by `std::default::Default::default`

error: aborting due to previous error; 1 warning emitted

MikailBag · 2020-07-12T18:42:28Z

@lcnr thanks, it's really serious argument against this combined "macro + const-generics" approach, especially the last example.

Let me summarize how I see situation.
Only unbroken (i.e. playing well with compiler, generics and so on) approach is marker trait.
Only efficient approach is one that proposed now (using macros for small lengths).

shepmaster · 2020-07-12T18:53:45Z

Wait until MaybeUninit performance problems are solved.

I feel like this is the best final solution, and it would probably benefit multiple other cases.

MikailBag · 2020-10-10T22:07:58Z

Unfortunately, PR does not play well with min_specialization:

error: cannot specialize on trait `Default`
   --> library/core/src/array/mod.rs:392:5
    |
392 | /     impl<T: Default> DefaultHack for T {
393 | |         fn default_hack() -> Self {
394 | |             Default::default()
395 | |         }
396 | |     }
    | |_____^

I currently changed the feature from min_specialization to specialization.

RalfJung · 2020-10-10T23:17:33Z

I currently changed the feature from min_specialization to specialization.

Please do not. specialization is unsound. Some people worked hard to remove all uses of specialization from rustc and the stdlib.

MikailBag · 2020-10-10T23:20:49Z

Yeah, I understand that using non-min specialization is only workaround.

I have zero experience with trait system, so I can be wrong here.
As I see, min_specialization disallows any trait which is not marked with special attribute.
Docs say:

// We allow specializing on explicitly marked traits with no associated
// items.

And Default does not have associated types.

That's why I now think I should add this specialization marker to Default and min_specialization should work again.
I'll try this just now.

UPD: no, build fails:

 error: cannot specialize on trait `ArrayDefault`
   --> library/core/src/array/mod.rs:397:5
    |
397 | /     impl<T, const N: usize> Default for [T; N]
398 | |     where
399 | |         [T; N]: ArrayDefault,
400 | |     {
...   |
403 | |         }
404 | |     }
    | |_____^

error: specializing impl repeats parameter `T`
   --> library/core/src/lazy.rs:374:1
    |
374 | / impl<T: Default> Default for Lazy<T> {
375 | |     /// Creates a new lazy value using `Default` as the initializing function.
376 | |     fn default() -> Lazy<T> {
377 | |         Lazy::new(T::default)
378 | |     }
379 | | }
    | |_^

RalfJung · 2020-10-11T08:36:12Z

It is backwards-incompatible to add this marker to an existing trait. And it is unsound to do specialization on a trait unless the trait has that marker -- a trait that is used for specializing other traits needs to pass some stricter checks, which most existing traits will not pass. I don't know of a good way out of this, but I am also not a specialization expert. Cc @matthewjasper

Dylan-DPC-zz · 2020-10-31T12:57:28Z

This is backwards incompatible and looks highly likely won't be merged as is. If you find a different way to solve this, it is better to create a new PR. Thanks for taking the time to contribute

MikailBag · 2020-10-31T16:17:26Z

@Dylan-DPC can you please describe in what way this PR breaks compatibility?
I know there is a problem that PR currently uses an unsound specialization feature, but it is not backcompat concern AFAIU.

I don't know if the desired behavior can be achieved with min_specialization. I hope that it is possible: specialization usage here does not look too complex. I was waiting for WG-traits input on this PR.

nbdd0121 · 2021-06-19T00:04:48Z

Can't we just introduce a new MarkerDefault trait with #[rustc_unsafe_specialization_marker]? It's similar to the MarkerEq trait used in slice/cmp.rs:

#[rustc_unsafe_specialization_marker]
trait MarkerDefault: Default {}
impl<T: Default> MarkerDefault for T {}

trait DefaultHack  {
    fn default_hack() -> Self;
}

impl<T> DefaultHack for T {
    default fn default_hack() -> T {
        unreachable!();
    }
}

impl<T: MarkerDefault> DefaultHack  for T {
    fn default_hack() -> T {
        Default::default()
    }
}

RalfJung · 2021-06-19T08:39:19Z

We should IMO not introduce more rustc_unsafe_specialization_marker until we know how to make them safe.

crlf0710 · 2021-06-19T11:18:11Z

There's a specialization-free approach, waiting for the feature const_evaluatable_checked. See sample code in #86259 .

rust-highfive assigned dtolnay Jul 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2020

MikailBag marked this pull request as draft July 11, 2020 19:36

MikailBag marked this pull request as ready for review July 11, 2020 22:13

MikailBag mentioned this pull request Jul 11, 2020

Use const generics in impl Default for arrays #71690

Closed

2 tasks

dtolnay reviewed Jul 11, 2020

View reviewed changes

eddyb reviewed Jul 12, 2020

View reviewed changes

src/libcore/array/mod.rs Outdated Show resolved Hide resolved

eddyb reviewed Jul 12, 2020

View reviewed changes

src/libcore/array/mod.rs Outdated Show resolved Hide resolved

eddyb reviewed Jul 12, 2020

View reviewed changes

src/libcore/array/mod.rs Outdated Show resolved Hide resolved

kpp reviewed Jul 12, 2020

View reviewed changes

shepmaster reviewed Jul 12, 2020

View reviewed changes

dtolnay reviewed Jul 12, 2020

View reviewed changes

dtolnay mentioned this pull request Jul 12, 2020

MaybeUninit::assume_init optimizes poorly #74267

Open

MikailBag added 6 commits October 11, 2020 00:57

fix error

5353301

not bootstrap

bfa075c

unopt

b73556b

fix tidy

e373144

no need to go unsafe lol

6942111

fmt

6ddaadd

MikailBag force-pushed the default branch from 2cbc284 to 767ba55 Compare October 10, 2020 21:59

MikailBag added 3 commits October 11, 2020 02:10

experiment

55b208f

unfortunate

d2ca70b

rm garbage

fd23a86

MikailBag force-pushed the default branch from a5a60ae to fd23a86 Compare October 10, 2020 23:10

min spec

f786fa0

camelid 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 Oct 30, 2020

Dylan-DPC-zz closed this Oct 31, 2020

Gankra mentioned this pull request May 26, 2022

Implement Default for all array sizes #88744

Closed

memoryruins mentioned this pull request Mar 10, 2023

I found a beautiful gem of the most hideous code in the stdlib I've ever seen... and I cannot fully seem to justify it. #109006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default for arrays via const generics #74254

Default for arrays via const generics #74254

MikailBag commented Jul 11, 2020 •

edited

Loading

rust-highfive commented Jul 11, 2020

MikailBag commented Jul 11, 2020 •

edited

Loading

dtolnay left a comment

dtolnay commented Jul 11, 2020

bors commented Jul 11, 2020

bors commented Jul 12, 2020

lcnr commented Jul 12, 2020

kpp Jul 12, 2020

lcnr commented Jul 12, 2020 •

edited

Loading

shepmaster Jul 12, 2020

MikailBag Jul 12, 2020 •

edited

Loading

shepmaster Jul 12, 2020

MikailBag Jul 12, 2020

dtolnay left a comment

lcnr commented Jul 12, 2020

MikailBag commented Jul 12, 2020

dtolnay commented Jul 12, 2020

shepmaster commented Jul 12, 2020

MikailBag commented Jul 12, 2020

lcnr commented Jul 12, 2020 •

edited

Loading

lcnr commented Jul 12, 2020

MikailBag commented Jul 12, 2020 •

edited

Loading

shepmaster commented Jul 12, 2020

MikailBag commented Oct 10, 2020

RalfJung commented Oct 10, 2020

MikailBag commented Oct 10, 2020 •

edited

Loading

RalfJung commented Oct 11, 2020

Dylan-DPC-zz commented Oct 31, 2020

MikailBag commented Oct 31, 2020 •

edited

Loading

nbdd0121 commented Jun 19, 2021

RalfJung commented Jun 19, 2021

crlf0710 commented Jun 19, 2021

Default for arrays via const generics #74254

Default for arrays via const generics #74254

Conversation

MikailBag commented Jul 11, 2020 • edited Loading

rust-highfive commented Jul 11, 2020

MikailBag commented Jul 11, 2020 • edited Loading

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jul 11, 2020

bors commented Jul 11, 2020

bors commented Jul 12, 2020

lcnr commented Jul 12, 2020

kpp Jul 12, 2020

Choose a reason for hiding this comment

lcnr commented Jul 12, 2020 • edited Loading

shepmaster Jul 12, 2020

Choose a reason for hiding this comment

MikailBag Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

shepmaster Jul 12, 2020

Choose a reason for hiding this comment

MikailBag Jul 12, 2020

Choose a reason for hiding this comment

dtolnay left a comment

Choose a reason for hiding this comment

lcnr commented Jul 12, 2020

MikailBag commented Jul 12, 2020

dtolnay commented Jul 12, 2020

shepmaster commented Jul 12, 2020

MikailBag commented Jul 12, 2020

lcnr commented Jul 12, 2020 • edited Loading

lcnr commented Jul 12, 2020

MikailBag commented Jul 12, 2020 • edited Loading

shepmaster commented Jul 12, 2020

MikailBag commented Oct 10, 2020

RalfJung commented Oct 10, 2020

MikailBag commented Oct 10, 2020 • edited Loading

RalfJung commented Oct 11, 2020

Dylan-DPC-zz commented Oct 31, 2020

MikailBag commented Oct 31, 2020 • edited Loading

nbdd0121 commented Jun 19, 2021

RalfJung commented Jun 19, 2021

crlf0710 commented Jun 19, 2021

MikailBag commented Jul 11, 2020 •

edited

Loading

MikailBag commented Jul 11, 2020 •

edited

Loading

lcnr commented Jul 12, 2020 •

edited

Loading

MikailBag Jul 12, 2020 •

edited

Loading

lcnr commented Jul 12, 2020 •

edited

Loading

MikailBag commented Jul 12, 2020 •

edited

Loading

MikailBag commented Oct 10, 2020 •

edited

Loading

MikailBag commented Oct 31, 2020 •

edited

Loading