rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.62k stars 12.48k forks source link

Remove unreachable constant data from binary #104590

Open DzenanJupic opened 1 year ago

DzenanJupic commented 1 year ago

Take the following code i.e.:

// main.rs
const VAL: &[u8] = {
    ArrayVec::<u8, 1>::new()
        .push(42)
        .as_slice()
};

pub struct ArrayVec<T, const CAP: usize> {
    array: [std::mem::MaybeUninit<T>; CAP],
    len: usize,
}

impl<T: Copy, const CAP: usize> ArrayVec<T, CAP> {
    pub const fn new() -> Self { 
        Self {
            array: [std::mem::MaybeUninit::uninit(); CAP],
            len: 0,
        }
    }

    pub const fn push(mut self, value: T) -> Self {
        self.array[self.len] = std::mem::MaybeUninit::new(value);
        self.len += 1;
        self
    } 

    pub const fn as_slice(&self) -> &[T] {
        unsafe {
            std::slice::from_raw_parts(&self.array as *const _ as *const _, self.len)
        }
    }
}

fn main() {
    println!("{VAL:?}");
}

Running rustc -OOO ./main.rs results in a binary of size 4 MB. Increasing the capacity of the array to i.e. 100000000 results in a binary of size 100 MB, even though the resulting slice still only contains one element. The same happens with cargo and/or LTO enabled. Switching to a static does not help either.

It would be pretty nice if the compiler could remove this excess capacity from the resulting binary. Especially, since a similar pattern is often used when implementing constant/more powerful versions of stringify, concat, format, ...

I'm not sure, however, if that is considered backwards compatible since it's currently possible to access values outside the slice range within the array using get_unchecked. I would argue, that doing so is unsound since it relies on the compiler not performing a reasonable optimization.

Meta

rustc 1.67.0-nightly (83356b78c 2022-11-17)
binary: rustc
commit-hash: 83356b78c4ff3e7d84e977aa6143793545967301
commit-date: 2022-11-17
host: x86_64-unknown-linux-gnu
release: 1.67.0-nightly
LLVM version: 15.0.4
workingjubilee commented 1 year ago

I'm not sure, however, if that is considered backwards compatible since it's currently possible to access values outside the slice range within the array using get_unchecked.

Per get_unchecked's documentation:

Calling this method with an out-of-bounds index is undefined behavior even if the resulting reference is not used.

Those bounds are determined relative to the slice, not the datatype being sliced into. So no, that is not even slightly subject to the stability policy.

workingjubilee commented 1 year ago

No idea if this optimization can be performed, to be clear, but if there is a blocking reason, it's definitely not going to be because slice::get_unchecked exists, and is more likely to be technical than design.

thomcc commented 1 year ago

Whether or not we can do this is likely based on how https://github.com/rust-lang/unsafe-code-guidelines/issues/256 is resolved. I think for your case you should just copy to a smaller array though.

thomcc commented 1 year ago

Actually, looking at your code, the lifetime extension you're doing in as_slice is probably UB as-is.

RalfJung commented 1 year ago

We will almost certainly want to fix https://github.com/rust-lang/unsafe-code-guidelines/issues/256 by allowing access to neighboring bytes. So I don't think we can do this kind of shrinkage implicitly. It has to be opt-in from the user.

I would argue, that doing so is unsound since it relies on the compiler not performing a reasonable optimization.

I would argue the optimization is not reasonable since it breaks correct code. ;) (Using get_unchecked for this is wrong, it is documented to require an index within the bounds of the slice, but with add it probably should be okay.)

DzenanJupic commented 1 year ago

Actually, looking at your code, the lifetime extension you're doing in as_slice is probably UB as-is.

There should ideally be no lifetime extension, am I wrong about that? &[T] should get the same lifetime as &self, right? The only reason I'm using from_raw_parts is that range-indexing is not const yet.

DzenanJupic commented 1 year ago

I think for your case you should just copy to a smaller array though

This won't work for me, since I don't know the final array size 'at compile time' (it's still in a constant context I guess). My use case is concatenating associated constant slices into one slice. This can already be done today in stable, but only if all types are known. It does not yet work for generics (at least I did not get it to work, even with the generic_const_exprs feature)

Here is the example code, that does not work, that I posted in the r/rust questions thread a few weeks ago: playground

Here is how I eventually solved it: playground

DzenanJupic commented 1 year ago

We will almost certainly want to fix rust-lang/unsafe-code-guidelines#256 by allowing access to neighboring bytes. So I don't think we can do this kind of shrinkage implicitly. It has to be opt-in from the user.

Does rust-lang/unsafe-code-guidelines#256 apply to a const context though?

RalfJung commented 1 year ago

Does https://github.com/rust-lang/unsafe-code-guidelines/issues/256 apply to a const context though?

Yes, const-Rust is still (mostly) the same language as runtime-Rust -- differences only arise when absolutely necessary, e.g. because absolute pointer addresses are unknoweable.