Skip to content

Specialize arena alloc for cloned slices #81053

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

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 52 additions & 24 deletions compiler/rustc_arena/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,50 @@
#![cfg_attr(bootstrap, feature(min_const_generics))]
#![feature(min_specialization)]
#![cfg_attr(test, feature(test))]
#![feature(rustc_attrs)]

use smallvec::SmallVec;

use std::alloc::Layout;
use std::cell::{Cell, RefCell};
use std::cmp;
use std::iter::Cloned;
use std::marker::{PhantomData, Send};
use std::mem::{self, MaybeUninit};
use std::ptr;
use std::slice;
use std::slice::{self, Iter};

#[inline(never)]
#[cold]
pub fn cold_path<F: FnOnce() -> R, R>(f: F) -> R {
f()
}

/// Move the contents of an iterator to a contiguous memory region.
///
/// SAFETY: the caller must ensure that the destination memory is free and large enough to hold
/// the contents of the iterator. Must not be called for iterators that may allocate on the same
/// arena.
#[inline]
unsafe fn write_from_iter<'a, T, I: Iterator<Item = T>>(
mut iter: I,
len: usize,
mem: *mut T,
) -> &'a mut [T] {
let mut l = len;
for i in 0..len {
if let Some(value) = iter.next() {
std::ptr::write(mem.add(i), value);
} else {
l = i;
break;
}
}
// We only return as many items as the iterator gave us, even
// though it was supposed to give us `len`
return std::slice::from_raw_parts_mut(mem, l);
}

/// An arena that can hold objects of only one type.
pub struct TypedArena<T> {
/// A pointer to the next object to be allocated.
Expand Down Expand Up @@ -133,6 +160,10 @@ where
}
}

#[rustc_unsafe_specialization_marker]
trait Clonable: Clone {}
impl<T: Clone> Clonable for T {}
Copy link
Member

@the8472 the8472 Jan 15, 2021

Choose a reason for hiding this comment

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

My somewhat limited understanding of the min_specialization rules is that you're here promising that any specialization relying on Clonable will only be applied to types where T: Clone is not dependent lifetimes (directly or indirectly)
E.g. that there's no impl Clone for &'static Foo involved. That can only be guaranteed if you could also manually enumerate all T, i.e. they're not types supplied by external crates.

It would also mean that it's unsafe to use TypedArena for such types and users of it would have to uphold that in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure if this is the case, but I don't know the feature well enough. I was going by the example of FusedIterator and TrustedLen, both of which are implemented for e.g. std::slice::Iterator<'a, T: 'a> and I believe both are specialization marker traits.

Copy link
Member

Choose a reason for hiding this comment

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

The thing is that FusedIterator or TrustedLen are not implemented for arbitrary T. They're only implemented for structs controlled by std and those impl are only conditional on additional bounds that themselves are other specialization traits.

What's different here is that you're essentially declaring that any and all Clone types can act as a specialization marker for specializations relying on Clonable. This would only be valid when you have some way of reasoning about the set of T. And since T is what goes into TypedArena it means you've ensured that all uses of TypedArena are compatible with the min specialization rules. This kind of far-reaching obligation is why there's the word unsafe in that annotation.

But again, my understanding of min_specialization is quite limited, so I may be wrong here.

Copy link
Contributor Author

@bugadani bugadani Jan 15, 2021

Choose a reason for hiding this comment

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

All right, makes sense. I wish I had another way to specialize for Cloned<slice::Iter> :( Whatever else I tried, I got a conflicting implementation error....

But even then, perf may get back completely negative or neutral as well.


impl<T, const N: usize> IterExt<T> for std::array::IntoIter<T, N> {
#[inline]
fn alloc_from_iter(self, arena: &TypedArena<T>) -> &mut [T] {
Expand Down Expand Up @@ -167,6 +198,25 @@ impl<T> IterExt<T> for Vec<T> {
}
}

impl<'a, T: 'a> IterExt<T> for Cloned<Iter<'a, T>>
where
T: Clonable,
{
#[inline]
fn alloc_from_iter(self, arena: &TypedArena<T>) -> &mut [T] {
let len = self.len();
if len == 0 {
return &mut [];
}
// Move the content to the arena by copying and then forgetting it
unsafe {
let len = self.len();
let start_ptr = arena.alloc_raw_slice(len);
write_from_iter(self, len, start_ptr)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to rely on anything specific to Cloned. It just relies on the length. Wouldn't specializing on TrustedLen be sufficient here, considering that's already a specialization trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it wouldn't compile unfortunately, or at least I couldn't get it to. Cloned needs Clone on T.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, just specializing on anything TrustedLen is unsound, see #77945 for that. In short, Map is TrustedLen and can allocate on the same arena, which makes handling a panic safely an impossible nightmare of dropping uninitialized memory.

I went through the uses of alloc_from_iter and I found that most of those are Map and FlatMap, but some places allocate from Clonedslice::Iter so hence the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I meant impl<I: TrustedLen> IterExt for I. That'll give you the length.

Copy link
Member

Choose a reason for hiding this comment

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

Well it's closed already. But for the record, you might have been able to use use impl<I: TrustedLen + IsClonedSliceIterMarker> for I instead where the latter bound is a safe specialization marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this seems to compile:

#[rustc_specialization_trait]
trait IsClonedSliceIterMarker {}
impl<T> IsClonedSliceIterMarker for Cloned<Iter<'_, T>> {}

impl<T, I> IterExt<T> for I
where
    I: IntoIterator<Item = T> + IsClonedSliceIterMarker + TrustedLen,
{
    #[inline]
    fn alloc_from_iter(self, arena: &TypedArena<T>) -> &mut [T] {
        let iter = self.into_iter();
        let len = iter.size_hint().0;
        if len == 0 {
            return &mut [];
        }
        // Move the content to the arena by copying and then forgetting it
        unsafe {
            let start_ptr = arena.alloc_raw_slice(len);
            write_from_iter(iter, len, start_ptr)
        }
    }
}

}

impl<A: smallvec::Array> IterExt<A::Item> for SmallVec<A> {
#[inline]
fn alloc_from_iter(mut self, arena: &TypedArena<A::Item>) -> &mut [A::Item] {
Expand Down Expand Up @@ -489,28 +539,6 @@ impl DroplessArena {
}
}

#[inline]
unsafe fn write_from_iter<T, I: Iterator<Item = T>>(
&self,
mut iter: I,
len: usize,
mem: *mut T,
) -> &mut [T] {
let mut i = 0;
// Use a manual loop since LLVM manages to optimize it better for
// slice iterators
loop {
let value = iter.next();
if i >= len || value.is_none() {
// We only return as many items as the iterator gave us, even
// though it was supposed to give us `len`
return slice::from_raw_parts_mut(mem, i);
}
ptr::write(mem.add(i), value.unwrap());
i += 1;
}
}

#[inline]
pub fn alloc_from_iter<T, I: IntoIterator<Item = T>>(&self, iter: I) -> &mut [T] {
let iter = iter.into_iter();
Expand All @@ -529,7 +557,7 @@ impl DroplessArena {
}

let mem = self.alloc_raw(Layout::array::<T>(len).unwrap()) as *mut T;
unsafe { self.write_from_iter(iter, len, mem) }
unsafe { write_from_iter(iter, len, mem) }
}
(_, _) => {
cold_path(move || -> &mut [T] {
Expand Down