Rust: Provide a trait method to (optionally) control resource allocation#1625
Rust: Provide a trait method to (optionally) control resource allocation#1625cpetig wants to merge 4 commits into
Conversation
cpetig
added
gen-rust
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
I like the idea, thanks! I've got some comments below, but let me know if you'd prefer to not handle anything in particular.
| r#"#[doc(hidden)] | ||
| /// Place the value on the heap or in an arena, return the raw pointer. | ||
| /// Override for custom resource allocators. | ||
| fn _resource_into_raw(val: Option<Self>) -> *mut Option<Self> where Self: Sized | ||
| {{ | ||
| {box_path}::into_raw({box_path}::new(val)) | ||
| }} | ||
|
|
||
| #[doc(hidden)] | ||
| /// Consumes the value from the handle, handle is invalid afterwards. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// See Box::from_raw | ||
| unsafe fn _resource_from_raw(handle: *mut Option<Self>) -> Option<Self> where Self: Sized | ||
| {{ | ||
| *unsafe {{ {box_path}::from_raw(handle) }} | ||
| }} |
There was a problem hiding this comment.
There's a few aspects of this I'd like to bikeshed if that's ok with you as well. One is that I'd rather not open-code the Option<Self> implementation if possible. Given how everything's moving to a trait-based definition, do you think it'd be possible to have, for example, a default associated type or something like that which represents the container here? Something like type RawContainer: SomeOtherTrait = Option<Self>; or something like that, where Option<T> by default implements SomeOtherTrait. That wouldn't be directly related to the allocation here, and would involve abstracting the preexisting .as_mut().unwrap() operations in a few places, but it'd be in the same trend as this.
Orthogonally I'd bikeshed these exact shapes/docs a bit. The _-prefixed methods are probably overkill at this point. It might make sense to rename them to maybe a _-suffix rather than a _ prefix (which typically means "yes I know this is unused ignore that" which isn't the case here). The implementations would stay the same, however. The documentation, though, I think will want to be expanded to explain a bit more about the allocation lifecycle and what's expected of the pointer (e.g. it lives for the entire duration of the resource). I think resource_into_raw will need to be unsafe as well because one of the contractual invariants of resource_from_raw will be that it only consumes values previously produced by resource_into_raw. Finally, the where Self: Sized I think is fine to move to the trait definition (make Sized a supertrait) and avoid the syntactic overhead here.
There was a problem hiding this comment.
I agree with hiding the Option<T> implementation detail by an associated type or similar (I think there exists an FooImp type or similar already for this). I am not sure about creating another trait here (feels overkill to me for now).
I also agree on the _ suffix and the better documentation and Sized in a different place.
But .. I consider the argumentation about into_raw better be unsafe not compelling. There is no invariant on into_raw, only from_raw has invariants (only called once and only on a result from into_raw). Similarly only one of these member functions is unsafe on Box as well.
There was a problem hiding this comment.
That sounds reasonable to me, yeah, and good points!
There was a problem hiding this comment.
I tried my best, but a lot of the really nice options require unstable associated type features.
There was a problem hiding this comment.
Please note that all the other types and functions in the trait are named _foo instead of foo_, so it feels a bit incoherent now.
| /// This is safe in single-threaded contexts. In multi-threaded contexts, | ||
| /// the caller must ensure exclusive access. | ||
| #[inline] | ||
| pub fn get_mut(&self) -> &mut Arena<T, SIZE> { |
There was a problem hiding this comment.
FWIW this is fundamentally unsafe and more-or-less UB. This'll want to be protected by some sort of lock or similar.
For this test though I think it's ok to avoid being fancy and just manually call Box maybe with a slightly different wrapper just to showcase that it can be done (e.g. wrap it up in Box<Thing<T>> instead of Box<T> or something like that)
There was a problem hiding this comment.
I agree. Turning & into &mut is instant UB - unless there is some sort of internal mutability involved (which probably needs an UnsafeCell to protect against compiler optimizations).
I didn't look that closely into this third re-implementation of the arena, my goal was to create a minimalist wasm component without any reference to malloc - which involves avoiding panic and libc, but can be achieved by wat-patching.
I guess I should look a bit more closely into how to make the arena throwaway example code opsem safe and not an evil example buried in a high-quality code base.
There was a problem hiding this comment.
Such a thing (&self to &mut T) exists ... https://doc.rust-lang.org/core/cell/struct.UnsafeCell.html#method.as_mut_unchecked but I agree that the unsafety should become better capsuled.
There was a problem hiding this comment.
Here I simplified the whole arena into an implementation I would be willing to defend the soundness of. I think it should be sound and not cause UB.
| contents: u32, | ||
| } | ||
|
|
||
| static ARENA: Arena<Option<MyThing>, 4> = Arena::new(); |
There was a problem hiding this comment.
Oh, this should become ThingStorage instead of Option
2 participants
This enables resource allocation from a user provided arena instead of the heap.
Arena implementation in test case generated by genAI.