Skip to content

Commit

Permalink
Rollup merge of #118094 - JarvisCraft:SpecFromElem-for-empty-tuple, r…
Browse files Browse the repository at this point in the history
…=thomcc

feat: specialize `SpecFromElem` for `()`

# Description

This PR adds a specialization `SpecFromElem for ()` which allows to significantly reduce `vec![(), N]` time in debug builds (specifically, tests) turning it from observable $O(n)$ to $O(1)$.

# Observing the change

The problem this PR aims to fix explicitly is slow `vec![(), N]` on big `N`s which may appear in tests (see [Background section](#Background) for more details).

The minimal example to see the problem:

```rust
#![feature(test)]

extern crate test;

#[cfg(test)]
mod tests {
    const HUGE_SIZE: usize = i32::MAX as usize + 1;

    #[bench]
    fn bench_vec_literal(b: &mut test::Bencher) {
        b.iter(|| vec![(); HUGE_SIZE]);
    }

    #[bench]
    fn bench_vec_repeat(b: &mut test::Bencher) {
        b.iter(|| [(); 1].repeat(HUGE_SIZE));
    }
}
```
<details><summary>Output</summary>
<p>

```bash
cargo +nightly test -- --report-time -Zunstable-options
   Compiling huge-zst-vec-literal-bench v0.1.0 (/home/progrm_jarvis/RustroverProjects/huge-zst-vec-literal-bench)
    Finished test [unoptimized + debuginfo] target(s) in 0.31s
     Running unittests src/lib.rs (target/debug/deps/huge_zst_vec_literal_bench-e43b1ef287ba8b36)

running 2 tests
test tests::bench_vec_repeat  ... ok <0.000s>
test tests::bench_vec_literal ... ok <14.382s>

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 14.38s

   Doc-tests huge-zst-vec-literal-bench

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
```
</p>
</details>

> [!IMPORTANT]
> This problem is only observable in Debug (unoptimized) builds, while Release (optimized) ones do not observe this problem. It still is worth fixing it, IMO, since the original scenario observes the problem in tests for which optimizations are disabled by default and it seems unreasonable to override this for the whole project while the problem is very local.

# Background

While working on a crate for a custom data format which has an `i32::MAX` limit on its list's sizes, I wrote the following test to ensure that this invariant is upheld:

```rust
#[test]
fn lists_should_have_i32_size() {
    assert!(
        RawNbtList::try_from(vec![(); i32::MAX as usize]).is_ok(),
        "lists should permit the size up to {}",
        i32::MAX
    );
    assert!(
        RawNbtList::try_from(vec![(); i32::MAX as usize + 1]).is_err(),
        "lists should have the size of at most {}",
        i32::MAX
    );
}
```

Soon I discovered that this takes $\approx 3--4s$ per assertion on my machine, almost all of which is spent on `vec![..]`.
While this would be logical for a non-ZST vector (which would require actual $O(n)$ allocation), here `()` was used intentionally considering that for ZSTs size-changing operations should anyway be $O(1)$ (at least from allocator perspective). Yet, this "overhead" is logical if we consider that in general case `clone()` (which is used by `Vec` literal) may have a non-trivial implementation and thus each element has to actually be visited (even if they occupy no space).

In my specific case, the following (contextual) equivalent solved the issue:

```rust
#[test]
fn lists_should_have_i32_size() {
    assert!(
        RawNbtList::try_from([(); 1].repeat(i32::MAX as usize)).is_ok(),
        "lists should permit the size up to {}",
        i32::MAX
    );
    assert!(
        RawNbtList::try_from([(); 1].repeat(i32::MAX as usize + 1)).is_err(),
        "lists should have the size of at most {}",
        i32::MAX
    );
}
```

which works since `repeat` explicitly uses `T: Copy` and so does not have to think about non-trivial `Clone`.

But it still may be counter-intuitive for users to observe such long time on the "canonical" vec literal thus the PR.

# Generic solution

This change is explicitly non-generic. Initially I considered it possible to implement in generically, but this would require the specialization to have the following type requirements:
- ✅ the type must be a ZST: easily done via
  ```rust
  if core::mem::size_of::<T>() == 0 {
    todo!("specialization")
  }
  ```
  or
  ```rust
  use core::mem::SizedTypeProperties;
  if T::IS_ZST {
    todo!("specialization")
  }
  ```
- :white_check_mark`: the type must implement `Copy`: implementable non-conflictable via a separate specialization:
  ```rust
  trait IsCopyZst: Sized {
    fn is_copy_zst() -> bool;
  }
  impl<T> IsCopyZst for T {
    fn is_copy_zst() -> bool {
        false
    }
  }
  impl<T: Copy> IsCopyZst for T {
    fn is_copy_zst() -> bool {
        Self::IS_ZST
    }
  }
  ```
- ❌ the type should have a trivial `Clone` implementation: since `vec![t; n]` is specified to use `clone()`, we can only use this "performance optimization" when we are guaranteed that `clone()` does nothing except for copying.

The latter is the real blocker for a generic fix since I am unaware of any way to get this information in a compiler-guaranteed way.

While there may be a fix for this (my friend `@CertainLach` has suggested a potential solution by an perma-unstable fn in `Clone` like `is_trivially_cloneable()` defaulting to `false` and only overridable by `rustc` on derive), this is surely out of this PRs scope.
  • Loading branch information
Nilstrieb committed Nov 21, 2023
2 parents 675cba0 + 72a8633 commit bfc2675
Showing 1 changed file with 19 additions and 4 deletions.
23 changes: 19 additions & 4 deletions library/alloc/src/vec/spec_from_elem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ impl SpecFromElem for i8 {
if elem == 0 {
return Vec { buf: RawVec::with_capacity_zeroed_in(n, alloc), len: n };
}
let mut v = Vec::with_capacity_in(n, alloc);
unsafe {
let mut v = Vec::with_capacity_in(n, alloc);
ptr::write_bytes(v.as_mut_ptr(), elem as u8, n);
v.set_len(n);
v
}
v
}
}

Expand All @@ -51,11 +51,26 @@ impl SpecFromElem for u8 {
if elem == 0 {
return Vec { buf: RawVec::with_capacity_zeroed_in(n, alloc), len: n };
}
let mut v = Vec::with_capacity_in(n, alloc);
unsafe {
let mut v = Vec::with_capacity_in(n, alloc);
ptr::write_bytes(v.as_mut_ptr(), elem, n);
v.set_len(n);
v
}
v
}
}

// A better way would be to implement this for all ZSTs which are `Copy` and have trivial `Clone`
// but the latter cannot be detected currently
impl SpecFromElem for () {
#[inline]
fn from_elem<A: Allocator>(_elem: (), n: usize, alloc: A) -> Vec<(), A> {
let mut v = Vec::with_capacity_in(n, alloc);
// SAFETY: the capacity has just been set to `n`
// and `()` is a ZST with trivial `Clone` implementation
unsafe {
v.set_len(n);
}
v
}
}

0 comments on commit bfc2675

Please sign in to comment.