Skip to content
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

Change the signatures of AllocRef to take self instead of &mut self #37

Closed
TimDiekmann opened this issue Jan 19, 2020 · 11 comments
Closed
Labels
A-Alloc Trait Issues regarding the Alloc trait Proposal

Comments

@TimDiekmann
Copy link
Member

TimDiekmann commented Jan 19, 2020

The gamedev WG made a proof of concept with our new AllocRef and BuildAllocRef trait (implemented at the alloc-wg crate) by implementing them for bumpalo.

It turned out, as mentioned in TimDiekmann/alloc-wg#11, that taking &mut self isn't always the best way to implement AllocRef. I would like to recall once again that AllocRef should not be implemented on stateful structs. That was the reason why we renamed Alloc to AllocRef 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 and System)
  • For non-ZST and non-Copy types, the user can chose between implementing it on &MyAllocator and &mut MyAllocator This does not work at all -> Closed
  • A oneshot allocator is trivially implementable
  • Probably solves Implementing AllocRef for actual references #34

The resulting API would look like this:

pub trait BuildAllocRef: Sized {
    type Ref: DeallocRef<BuildAlloc = Self>;

    unsafe fn build_alloc_ref(
        &mut self, // Stick with `&mut self` or change this to `self` as well?
        ptr: NonNull<u8>,
        layout: Option<NonZeroLayout>,
    ) -> Self::Ref;
}

pub trait DeallocRef: Sized + Copy {
    type BuildAlloc: BuildAllocRef<Ref = Self>;

    fn get_build_alloc(self) -> Self::BuildAlloc;

    unsafe fn dealloc(self, ptr: NonNull<u8>, layout: NonZeroLayout);
}


pub trait AllocRef: DeallocRef {
    type Error;

    fn alloc(self, layout: NonZeroLayout) -> Result<NonNull<u8>, Self::Error>;

    // ...
}

pub trait ReallocRef: AllocRef {
    unsafe fn realloc(
        self,
        ptr: NonNull<u8>,
        old_layout: NonZeroLayout,
        new_layout: NonZeroLayout,
    ) -> Result<NonNull<u8>, Self::Error> {
        // ...
    } 
}
@gnzlbg
Copy link

gnzlbg commented Jan 20, 2020

This makes sense to me. Also:

A oneshot allocator is trivially implementable

This is cool.

@CAD97
Copy link

CAD97 commented Mar 12, 2020

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].)

@TimDiekmann
Copy link
Member Author

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].)

Yes, that's true. AllocRef would be implemented on &T then:

unsafe impl<T: GlobalAlloc> AllocRef for &T { ... }

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Mar 12, 2020

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...

@TimDiekmann TimDiekmann mentioned this issue Mar 12, 2020
18 tasks
@TimDiekmann
Copy link
Member Author

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);
    }
}

@CAD97
Copy link

CAD97 commented Mar 12, 2020

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.

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Mar 28, 2020

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 implement Copy). This means in practice all allocators will need to Clone (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 do impl<'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.

@TimDiekmann
Copy link
Member Author

For an example how to implement shareable allocators please see the example in the RFC.

@Wodann
Copy link

Wodann commented Mar 28, 2020

Works for me 👌🏻

@JelteF
Copy link

JelteF commented Mar 28, 2020

@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.

@TimDiekmann
Copy link
Member Author

TimDiekmann commented Mar 28, 2020

@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`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Alloc Trait Issues regarding the Alloc trait Proposal
Projects
None yet
Development

No branches or pull requests

5 participants