Closed TimDiekmann closed 4 years ago
This makes sense to me. Also:
A oneshot allocator is trivially implementable
This is cool.
Just a small docs note: if the methods take self
, the following line should probably be changed:
AllocRef
is designed to be implemented on ZSTs, references, or smart pointers
because implementing AllocRef
for a type like Arc<MyAllocImpl>
would probably be a poor choice, because any call to a method would require a temporary +1 on the reference count. Instead, it should probably just say "ZSTs and references".
It should also be noted that methods by self
makes the trait not object-safe, which means no dynamic allocator selection with AllocRef
. (This includes #[global_allocator]
.)
It should also be noted that methods by
self
makes the trait not object-safe, which means no dynamic allocator selection withAllocRef
. (This includes#[global_allocator]
.)
Yes, that's true. AllocRef
would be implemented on &T
then:
unsafe impl<T: GlobalAlloc> AllocRef for &T { ... }
it should probably just say "ZSTs and references"
Unless the smart pointers are Copy
it's not possible to implement it on them anyway as this change requires T: Copy
. Otherwise the default implementation for grow
and shrink
are not possible. Sure, we could also drop the default implementation...
At least at the playground this works quite well:
fn test_alloc<T, A: GlobalAlloc>(alloc: A) {
let layout = Layout::new::<T>();
let p = AllocRef::alloc(&alloc, layout).expect("Failed to allocate");
unsafe {
AllocRef::dealloc(&alloc, p, layout);
}
}
Yes, it definitely will work (and will continue working) when proxied through GlobalAlloc
. Though perhaps the example should use &dyn GlobalAlloc
since that's roughly what alloc::Global
is.
What I'm raising a concern for is if/when GlobalAlloc
is deprecated in favor of using AllocRef
directly. If we go for "AllocRef
for static local allocators, GlobalAlloc
for dynamic allocators" or similar, this is fine.
I experimented a lot with &mut self
, &self
, and self
at alloc-wg
and alloc-compose
. It turns out, that only &mut self
is a viable option here as soon as we get to allocators, which are not shareable. I like to quote the initial RFC here:
if you want to use an allocator for more than one allocation, you will need to call
clone()
(or make the allocator parameter implementCopy
). This means in practice all allocators will need toClone
(and thus support sharing in general, as in the Allocators and lifetimes section).Put more simply, requiring that allocators implement
Clone
means it will not be pratical to doimpl<'a> Allocator for &'a mut MyUniqueAlloc
.
Even without the last point, collections like Vec<T, A: AllocRef>
can't know if A
is a is a mut
-reference and requires a lot of cloning.
For an example how to implement shareable allocators please see the example in the RFC.
Works for me 👌🏻
@TimDiekmann Shouldn't the problem you mention be solved by the fact that you require DeallocRef
to be Copy
(as shown in the trait definition in your first post in this issue). I'm probably misunderstandig it, but I don't see how the section from the original would still apply in that case. There would be no need for calling clone on the Allocator, since it is defined to be Copy
.
@JelteF &mut T
does not implement Copy
, thus impl AllocRef for &mut MyAllocator
does not work at all.
trait AllocRef: Copy {}
#[derive(Copy, Clone)]
struct MyAlloc {}
// works
impl AllocRef for MyAlloc {}
// E0277
impl AllocRef for &mut MyAlloc {}
error[E0277]: the trait bound `&mut MyAlloc: std::marker::Copy` is not satisfied
--> src/lib.rs:10:6
|
10 | impl AllocRef for &mut MyAlloc {}
| ^^^^^^^^ the trait `std::marker::Copy` is not implemented for `&mut MyAlloc`
|
= help: the following implementations were found:
<MyAlloc as std::marker::Copy>
= note: `std::marker::Copy` is implemented for `&MyAlloc`, but not for `&mut MyAlloc`
The gamedev WG made a proof of concept with our new
AllocRef
andBuildAllocRef
trait (implemented at thealloc-wg
crate) by implementing them forbumpalo
.It turned out, as mentioned in https://github.com/TimDiekmann/alloc-wg/pull/11, that taking
&mut self
isn't always the best way to implementAllocRef
. I would like to recall once again thatAllocRef
should not be implemented on stateful structs. That was the reason why we renamedAlloc
toAllocRef
in #8.@Wodann and I discussed a bit about this problem in the gamedev discord channel and my suggestion is to take
self
instead of&mut self
. This solves several issues:AllocRef
can be implemented on ZST types as before (e.g.Global
andSystem
)Copy
types, the user can chose between implementing it on&MyAllocator
and&mut MyAllocator
~ This does not work at all -> ClosedThe resulting API would look like this: