-
Notifications
You must be signed in to change notification settings - Fork 275
Rust: Provide a trait method to (optionally) control resource allocation #1625
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
Open
cpetig
wants to merge
4
commits into
bytecodealliance:main
Choose a base branch
from
cpetig:resource_arena
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+170
−5
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| include!(env!("BINDINGS")); | ||
|
|
||
| use crate::test::arena_allocated_resources::to_test::Thing; | ||
|
|
||
| struct Component; | ||
|
|
||
| export!(Component); | ||
|
|
||
| impl Guest for Component { | ||
| fn run() { | ||
| let thing1 = Thing::new(3); | ||
| let thing2 = Thing::new(5); | ||
| assert_eq!(3, thing1.get()); | ||
| assert_eq!(5, thing2.get()); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| include!(env!("BINDINGS")); | ||
|
|
||
| use crate::exports::test::arena_allocated_resources::to_test::{Guest, GuestThing, ThingStorage}; | ||
|
|
||
| export!(Component); | ||
|
|
||
| struct Component; | ||
|
|
||
| impl Guest for Component { | ||
| type Thing = MyThing; | ||
| } | ||
|
|
||
| mod arena { | ||
|
|
||
| use core::sync::atomic::{AtomicUsize, Ordering}; | ||
| use core::{cell::UnsafeCell, mem::MaybeUninit}; | ||
|
|
||
| /// A simple no_std arena allocator for fixed-size allocations. | ||
| /// | ||
| /// The arena allocates items of type T sequentially from a pre-allocated buffer | ||
| /// and does not support individual deallocation. Memory is reclaimed | ||
| /// only when the entire arena is reset. | ||
| pub struct Arena<T, const SIZE: usize> { | ||
| buffer: [UnsafeCell<MaybeUninit<T>>; SIZE], | ||
| offset: AtomicUsize, | ||
| } | ||
|
|
||
| // Element allocation is atomic and elements are exclusively handed out after allocation, | ||
| // so the arena can be send to other threads and simultaneosly accessed by multiple threads | ||
| unsafe impl<T: Sync, const SIZE: usize> Sync for Arena<T, SIZE> {} | ||
| unsafe impl<T: Send, const SIZE: usize> Send for Arena<T, SIZE> {} | ||
|
|
||
| impl<T: Default, const SIZE: usize> Arena<T, SIZE> { | ||
| pub const fn new() -> Self { | ||
| Self { | ||
| buffer: [const { UnsafeCell::new(MaybeUninit::uninit()) }; SIZE], | ||
| offset: AtomicUsize::new(0), | ||
| } | ||
| } | ||
|
|
||
| /// Allocates space for a single item of type T. | ||
| /// Returns a mutable reference to the allocated memory, or None if there's insufficient space. | ||
| pub fn alloc_one(&self) -> Option<&mut T> { | ||
| // short circuit the exhausted state (don't increment if full) | ||
| if self.offset.load(Ordering::Relaxed) >= SIZE { | ||
| None | ||
| } else { | ||
| // now try to allocate for real | ||
| let pos = self.offset.fetch_add(1, Ordering::Acquire); | ||
| if pos >= SIZE { | ||
| // now self.offset is already beyond SIZE, reduce our increment and return none | ||
| self.offset.fetch_sub(1, Ordering::Release); | ||
| None | ||
| } else { | ||
| let ptr = self.buffer[pos].get(); | ||
| // SAFETY: we demand exclusive ownership of the item in the arena | ||
| let uninit = unsafe { &mut *ptr }; | ||
| Some(uninit.write(Default::default())) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| use arena::Arena; | ||
|
|
||
| #[derive(Clone)] | ||
| struct MyThing { | ||
| contents: u32, | ||
| } | ||
|
|
||
| static ARENA: Arena<Option<MyThing>, 4> = Arena::new(); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this should become ThingStorage instead of Option |
||
|
|
||
| impl GuestThing for MyThing { | ||
| fn new(v: u32) -> MyThing { | ||
| MyThing { contents: v } | ||
| } | ||
|
|
||
| fn get(&self) -> u32 { | ||
| self.contents | ||
| } | ||
|
|
||
| fn resource_into_raw_(val: ThingStorage<Self>) -> *mut ThingStorage<Self> | ||
| where | ||
| Self: Sized, | ||
| { | ||
| val.and_then(|v| { | ||
| ARENA.alloc_one().map(|x| { | ||
| *x = Some(v); | ||
| x as *mut _ | ||
| }) | ||
| }) | ||
| .unwrap_or(core::ptr::null_mut()) | ||
| } | ||
|
|
||
| unsafe fn resource_from_raw_(handle: *mut ThingStorage<Self>) -> ThingStorage<Self> | ||
| where | ||
| Self: Sized, | ||
| { | ||
| let res = unsafe { &mut *handle }.take(); | ||
| res | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package test:arena-allocated-resources; | ||
|
|
||
| interface to-test { | ||
| resource thing { | ||
| constructor(v: u32); | ||
| get: func() -> u32; | ||
| } | ||
| } | ||
|
|
||
| world test { | ||
| export to-test; | ||
| } | ||
|
|
||
| world runner { | ||
| import to-test; | ||
|
|
||
| export run: func(); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 atrait-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 liketype RawContainer: SomeOtherTrait = Option<Self>;or something like that, whereOption<T>by default implementsSomeOtherTrait. 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 thinkresource_into_rawwill need to beunsafeas well because one of the contractual invariants ofresource_from_rawwill be that it only consumes values previously produced byresource_into_raw. Finally, thewhere Self: SizedI think is fine to move to the trait definition (makeSizeda supertrait) and avoid the syntactic overhead here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with hiding the
Option<T>implementation detail by an associated type or similar (I think there exists anFooImptype 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_rawbetter beunsafenot compelling. There is no invariant oninto_raw, onlyfrom_rawhas invariants (only called once and only on a result frominto_raw). Similarly only one of these member functions is unsafe on Box as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable to me, yeah, and good points!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my best, but a lot of the really nice options require unstable associated type features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that all the other types and functions in the trait are named
_fooinstead offoo_, so it feels a bit incoherent now.