feat(allocator): add Vec::into_boxed_slice

[DonIsaac]

Note that this PR does not implement the inverse operation (Box::to_vec for [T]).

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[DonIsaac]

• feat(allocator): add Vec::into_boxed_slice #6195 👈
• perf(allocator): use lower bound of size hint when creating Vecs from an iterator #6194
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #6195 will not alter performance

_{Comparing don/09-30-feat_allocator_add_vec_into_boxed_slice_ (e14701c) with main (5db9b30)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Hmm. I'm not sure about this.

I'm not that familiar with allocator_api2's implementation of into_boxed_slice, but it sounds like it can reallocate (copy all the data), which we definitely don't want.

That makes sense for allocator_api2 as it's generic over any kind of allocator, but in our case we're using a bump allocator which cannot recover excess capacity, so it's pointless to ever copy the data.

I don't know if bumpalo::Bump would automatically ignore the request to reallocate or not. Would you like to look into that?

Alternatively, we could get the pointer and length of the Vec and construct a NonNull<[T]> manually. That'd definitely avoid memory copies. (Note: would need to wrap the Vec in a ManuallyDrop first, as allocator_api2's Vec is Drop)

This all requires quite a bit of care. I'm not completely up to speed on the safety constraints. Sometimes there are non-obvious subtleties to avoiding UB, and I usually end up having to spend a few hours boning up on arcane details before I'm sure (well, at least reasonably sure) that I've not committed some UB crime!

Do you have a concrete use case for wanting a boxed slice to data in the arena?

[overlookmotel]

By the way, we're planning to replace this Vec implementation with a hand-written one with length and capacity fields as u32s, to bring size of Vec down from 32 bytes to 24 bytes (oxc-project/backlog#18). Dunqing was going to look at doing that fairly soon.

So at that point, we wouldn't be able to use allocator_api2's into_boxed_slice anyway.

[DonIsaac]

https://github.com/zakarumych/allocator-api2/blob/f937b9650e10137f782698bd4eb4f8b36f1817a7/src/stable/vec/mod.rs#L1046

I don't think it creates a new allocation. I want this because it cuts the size of a dynamically sized vec in half (32 -> 16 bytes) at the cost of not being re-sizeable. I'm working on a prototype where this is useful.

[overlookmotel]

Docs for into_boxed_slice specifically say "If the vector has excess capacity, its items will be moved into a newly-allocated buffer with exactly the right capacity." i.e. memory copy.

into_boxed_slice calls shrink_to_fit, and shrink_to_fit can cause a memory copy. Or maybe it doesn't if Bumpalo chooses not to, but to determine that conclusively would require tracing the calls all the way down to Bump::shrink.

At the very least it invokes quite a lot of complex machinery, which may not get elided.

Ideally we'd do this instead:

pub fn into_boxed_slice(self) -> Box<'alloc, [T]> {
    let (ptr, len, _) = self.0.into_raw_parts();
    let ptr = unsafe { NonNull::new_unchecked(ptr) };
    let ptr = NonNull::slice_from_raw_parts(ptr, len);
    unsafe { Box::from_non_null(ptr) }
}

But I don't think we can, because Vec::into_raw_parts does not guarantee the pointer it returns is non-null. In practice it is, because RawVec contains a NonNull, but that's an internal implementation detail of Vec, and no guarantee it won't change.

[overlookmotel]

Actually, this works:

pub fn into_boxed_slice(self) -> Box<'alloc, [T]> {
    let slice = self.0.leak();
    let ptr = NonNull::from(slice);
    // SAFETY: `ptr` points to a valid slice `[T]`.
    // Lifetime of returned `Box<'alloc, [T]>` is same as lifetime of consumed `Vec<'alloc, T>`,
    // so data in the `Box` must be valid for its lifetime.
    unsafe { Box::from_non_null(ptr) }
}

We can use Vec::leak because in our use case, the types stored in arena are all non-Drop.

Then you can remove the comment about "If the vector has excess capacity, its items will be moved into a newly-allocated buffer with exactly the right capacity."

[overlookmotel]

This comment should be updated. It's not true that the pointer has to be from another Box. If that was the case, then Vec::into_boxed_slice which you're adding here would violate the contract, as it's not a Box!

The requirement is that pointer needs to point to data in same arena (could be from a Vec, a String, anything else).

Actually, not even that is quite right. Intended usage is that it be data from the same arena, but the safety constraint I think is a bit looser - it can point to any data as long as that data outlives 'alloc.

There's also a safety requirement about no aliasing.

And the NonNull<T> must have been created from a mutable pointer *mut T, not a *const T, or that would be UB too.

This may all seem like nitpicking, but in my view it's vitally important that documentation for unsafe methods is accurate about what the contract is, otherwise it's impossible to verify the method is being used correctly.

[DonIsaac]

@overlookmotel does it make sense to just abandon this PR?

[overlookmotel]

No, I think we can do it. I just think better to use this version:

pub fn into_boxed_slice(self) -> Box<'alloc, [T]> {
    let slice = self.0.leak();
    let ptr = NonNull::from(slice);
    // SAFETY: `ptr` points to a valid slice `[T]`.
    // Lifetime of returned `Box<'alloc, [T]>` is same as lifetime of consumed `Vec<'alloc, T>`,
    // so data in the `Box` will be valid for its lifetime.
    unsafe { Box::from_non_null(ptr) }
}

It's safe, and won't copy memory.