-
Notifications
You must be signed in to change notification settings - Fork 78
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
mini-alloc: a very simple bump allocator #87
Conversation
2d1cccf
to
47c93f1
Compare
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.
Nice work! This is def going to make binaries smaller :)
A few things
mini-alloc/tests/wasm.rs
Outdated
let p1 = Vec::<u8>::with_capacity(700); | ||
assert!(p1.as_ptr() as usize > 0); | ||
let p2 = Vec::<u8>::with_capacity(65536); | ||
assert_eq!(p1.as_ptr() as usize + 700, p2.as_ptr() as usize); |
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.
p1.as_ptr().add(700) == p2.as_ptr()
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.
This will require an unsafe
block. Is that still your preference?
mini-alloc/src/lib.rs
Outdated
} | ||
|
||
fn round_up_to_alignment(val: usize, align: usize) -> usize { | ||
(val + align - 1) & (-(align as isize) as usize) |
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.
Let's make this const
This isn't right for non power-of-two alignments
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.
align
is always a power of two in Layout
:
https://doc.rust-lang.org/std/alloc/struct.Layout.html
But I can add a comment on this function making it clear that align
must be a power of two.
Now supports unix and windows, with a completely separate implementation from wasm32. Addressed your other comments as well. I'd like to squash this before merging. |
Oh, also, the only windows testing I've done is |
a2a0c83
to
3953ba2
Compare
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.
Everything looks great! Only had minor comments
/// instructions than the positive offset. | ||
static mut STATE: Option<(NonZero, usize)> = None; | ||
|
||
unsafe fn alloc_impl(layout: Layout) -> Option<*mut u8> { |
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.
Hi @mikebenfield should we validate layout.size()
before doing more stuff here? Maybe check it is non-zero or not some excessive value
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.
It's the caller's responsibility to ensure the size is non-zero. Our behavior is undefined if we receive a zero size. See the method documentation.
As for an excessively large size, I think we already do the only reasonable thing we can do - if we need to grow more pages and can't, we fail. Is there another check you'd like to see?
mini-alloc/src/imp.rs
Outdated
unsafe fn alloc_impl(layout: Layout) -> Option<*mut u8> { | ||
let (neg_offset, neg_bound) = STATE.get_or_insert_with(|| { | ||
let heap_base = &__heap_base as *const u8 as usize; | ||
let bound = PAGE_SIZE * wasm32::memory_size(0); |
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.
Each time this occurs, it has to call wasm32::memory_size(0) which I believe won't change throughout the duration of a program. Is it possible to cache this?
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.
get_or_insert_with
will only call this closure the first time this is executed. After that STATE
will contain a Some
rather than a None
and the values will just be retrieved.
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.
Oh perfect, I missed that you are indeed caching the bound there
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've pushed a commit that adds a test for various edge cases. We're getting close!
Btw don't worry about force pushing :)
mini-alloc/src/imp.rs
Outdated
if wasm32::memory_grow(0, pages_needed) == usize::MAX { | ||
return None; | ||
} | ||
*neg_bound -= PAGE_SIZE * pages_needed; |
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.
This overflows if the max number of pages are allocated.
This is why in #89 we start the boundary 1 off.
781084e
to
bbad3d1
Compare
bbad3d1
to
9c53519
Compare
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.
Looks great! Nice work!
No description provided.