mini-alloc: a very simple bump allocator #87
Conversation
mikebenfield
force-pushed
the
mini-alloc3
branch
from
2d1cccf
to
47c93f1
Compare
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.
p1.as_ptr().add(700) == p2.as_ptr()
There was a problem hiding this comment.
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.
Let's make this const
This isn't right for non power-of-two alignments
There was a problem hiding this comment.
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 |
mikebenfield
force-pushed
the
mini-alloc3
branch
6 times, most recently
from
a2a0c83
to
3953ba2
Compare
| /// 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.
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.
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.
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.
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.
Oh perfect, I missed that you are indeed caching the bound there
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.
This overflows if the max number of pages are allocated.
This is why in #89 we start the boundary 1 off.
mikebenfield
force-pushed
the
mini-alloc3
branch
2 times, most recently
from
781084e
to
bbad3d1
Compare
mikebenfield
force-pushed
the
mini-alloc3
branch
from
bbad3d1
to
9c53519
Compare
No description provided.