Use new nightly GlobalAlloc API#11
Conversation
phil-opp
left a comment
phil-opp
left a comment
There was a problem hiding this comment.
Thanks a lot! I left a few comments, but overall this looks good to me.
I wonder why the return type of alloc was changed to a raw pointer instead of a Result? I couldn't find an explanation for it in the rust PRs/issues…
src/lib.rs
Outdated
| self.allocate_first_fit(layout) | ||
| unsafe impl GlobalAlloc for Heap { | ||
| unsafe fn alloc(&self, layout: Layout) -> *mut Opaque { | ||
| ALLOCATOR.allocate_first_fit(layout) |
There was a problem hiding this comment.
GlobalAlloc takes &self instead of &mut self, so as far as I know static and RefCell are the only choices here. There's also new Alloc trait, but it can't be used with #[global_allocator]. In that case someone using this library would have to implement GlobalAlloc on his own to be able to use it with #[global_allocator]
src/lib.rs
Outdated
| self.0.lock().allocate_first_fit(layout) | ||
| unsafe impl<'a> GlobalAlloc for &'a LockedHeap { | ||
| unsafe fn alloc(&self, layout: Layout) -> *mut Opaque { | ||
| LOCKED_ALLOCATOR.0.lock().allocate_first_fit(layout) |
There was a problem hiding this comment.
Why the new LOCKED_ALLOCATOR static?
There was a problem hiding this comment.
Same as with ALLOCATOR
src/test.rs
Outdated
| let layout = Layout::from_size_align(heap.size() + 1, align_of::<usize>()); | ||
| let addr = heap.allocate_first_fit(layout.unwrap()); | ||
| assert!(addr.is_err()); | ||
| assert!(addr as *mut u8 == core::ptr::null_mut()); |
There was a problem hiding this comment.
Maybe we can encapsulate the == core::ptr::null_mut() check in some is_err / is_ok functions to avoid the boilerplate? It might also make sense to move all the tests in an inline tests submodule.
| let mut heap = new_heap(); | ||
|
|
||
| let layout = Layout::from_size_align(size_of::<usize>() * 2, align_of::<usize>()).unwrap(); | ||
| let x = heap.allocate_first_fit(layout.clone()).unwrap(); |
There was a problem hiding this comment.
We shouldn't just remove the unwrap, because it also checked that the allocation succeeded. When we don't check it we risk deallocating a null pointer. Also applies to the other cases where unwraps are removed.
There was a problem hiding this comment.
Looks like I'll have to add asserts here, or write a function that panics when pointer is null
|
I think return type of alloc now uses raw pointer because it makes it easier to use allocators written in C. |
|
I thought a bit more about this and I really don't like the new C-style API. So how about keeping it the old way (i.e. implement the Alloc trait and return Results from almost all methods) and only update LockedHeap to create a GlobalAlloc on top of that? Then users can use LockedHeap as global allocator, use Heap as non-global allocator, or build an own global allocator on top of Heap. |
|
I implemented the changes you wanted, let me know if I should change anything else. |
phil-opp
left a comment
phil-opp
left a comment
There was a problem hiding this comment.
Thanks a lot, looks really good now!
I think we can remove the LOCKED_ALLOCATOR because we use a Mutex in LockedHeap.
src/lib.rs
Outdated
| self.0.lock().allocate_first_fit(layout) | ||
| unsafe impl<'a> GlobalAlloc for &'a LockedHeap { | ||
| unsafe fn alloc(&self, layout: Layout) -> *mut Opaque { | ||
| LOCKED_ALLOCATOR.0.lock().allocate_first_fit(layout).ok().map_or(0 as *mut Opaque, |allocation| { |
There was a problem hiding this comment.
I think we should be able to do just self.0.lock without the static LOCKED_ALLOCATOR.
There was a problem hiding this comment.
Yep, Mutex lock() uses &self and returns MutexGuard, so static is not needed here.
There was a problem hiding this comment.
Is the &'a needed in impl? It works without it.
|
I removed the LOCKED_ALLOCATOR static, also I removed &'a from impl |
|
Thanks a lot! The |
|
Released as version 0.6.0. |
2 participants
Current nightly requires the use of new GlobalAlloc trait with #[global_allocator].
You can find more info here: rust-lang/rust#49669