Add tests and change bump allocator implementation#25
Add tests and change bump allocator implementation#25Luro02 wants to merge 5 commits intosysgrok:masterfrom
Conversation
|
Nit: I'm not sure replacing pub struct BumpBox<'a, T> {
ptr: NonNull<T>,
_allocator: core::marker::PhantomData<&'a ()>,
}with pub struct BumpBox<'a, T> {
value: &'a mut T,
}... is necessarily making the intent clearer. However removing the destructor impl<T> Drop for BumpBox<'_, T> {
fn drop(&mut self) {
// Safety: The pointer is valid and we own the data
unsafe {
self.ptr.as_ptr().drop_in_place();
}
}
}... is absolutely NOK. Why did you do that? Otherwise, I totally like having |
It is OK (in that it will never, ever lead to a program crash or anything like that - it is just a UB if you read from the uninitialized memory which was assumed initialized, but we don't do that, and even if we did, it would be "undefined behavior" but not a crash).
Yeah. Maybe just |
I think I misunderstood a part of the code, yeah |
ivmarkov
left a comment
ivmarkov
left a comment
There was a problem hiding this comment.
See my two comments. Rest is fine.
src/bump.rs
Outdated
| const fn new() -> Self { | ||
| Self { | ||
| memory: MaybeUninit::uninit(), | ||
| memory: [const { MaybeUninit::uninit() }; N], |
There was a problem hiding this comment.
Have you confirmed that [const { MaybeUninit::uninit() }; N], would absolutely, positively NOT allocate on-stack and them move to the final destination?
To my understanding, only MaybeUninit::uninit() is guaranteed to have this property, regardless of the compiler optimization settings.
There was a problem hiding this comment.
I tried it out, and it worked fine, but based on my research, it seems like this is not always the case (seems to depend on compiler optimizations). Given that we are working with 1.77, the const expression is sadly unavailable.
|
PR #25: Size comparison from cf4a089 to 8ccbff4 Full report (2 builds for light, light_eth)
|
|
Hm, miri seems unhappy with your changes? How was it before your changes? |
|
The CI failure is what I referenced with this comment:
Here is what it outputs on master with the tests copied and the The fix here is // Safety: We just allocated the memory and it's properly aligned
let ptr = unsafe {
- let ptr = t_buf.as_ptr() as *mut T;
+ let ptr = t_buf.as_mut_ptr() as *mut T;
ptr.write(object);
NonNull::new_unchecked(ptr)
};and with that fix, miri will show the same problem on master that my PR has: The problem is this: It will return a pointer to the offsets Given that an I have tried a few things, but none of them made miri happy. |
|
I see. I played a bit with it today, and I think you are right - I'm not sure there is necessarily a bug in it (?) but I'm also not sure how to fix it so that Let me think about it a bit more in the next couple of days (a bit busy with other work ATM). |
2 participants
While looking into #24, I noticed some pieces of code in the bump allocator that looked wrong.
What has been changed?
let ptr = t_buf.as_ptr() as *mut T;where a*const Tis cast to*mut T&selfby the use of pointers, I made this explicit through references. Note that this might have been a wrong decision, miri isn't happy with my current impl (see pending CI), because of overlapping borrows. Will have to check if the original code had that issue too.