-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -133,6 +160,10 @@ where | |
} | ||
} | ||
|
||
#[rustc_unsafe_specialization_marker] | ||
trait Clonable: Clone {} | ||
impl<T: Clone> Clonable for T {} | ||
|
||
impl<T, const N: usize> IterExt<T> for std::array::IntoIter<T, N> { | ||
#[inline] | ||
fn alloc_from_iter(self, arena: &TypedArena<T>) -> &mut [T] { | ||
|
@@ -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) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to rely on anything specific to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] { | ||
|
@@ -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(); | ||
|
@@ -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] { | ||
|
There was a problem hiding this comment.
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 onClonable
will only be applied to types whereT: 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.There was a problem hiding this comment.
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
andTrustedLen
, both of which are implemented for e.g.std::slice::Iterator<'a, T: 'a>
and I believe both are specialization marker traits.There was a problem hiding this comment.
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
orTrustedLen
are not implemented for arbitraryT
. They're only implemented for structs controlled by std and thoseimpl
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 onClonable
. This would only be valid when you have some way of reasoning about the set ofT
. And sinceT
is what goes intoTypedArena
it means you've ensured that all uses ofTypedArena
are compatible with the min specialization rules. This kind of far-reaching obligation is why there's the wordunsafe
in that annotation.But again, my understanding of
min_specialization
is quite limited, so I may be wrong here.There was a problem hiding this comment.
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.