-
Notifications
You must be signed in to change notification settings - Fork 34
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
rework heap allocation api #93
Conversation
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.
You might also want to change Cargo.toml
and update the version
field. We haven't reached 1.0
yet, but I think we should get used to semantically version hvm-core
.
src/run/allocator.rs
Outdated
/// Allocates a new heap with at most the given size in bytes. | ||
pub fn new(bytes: Option<usize>) -> Option<Box<Self>> { |
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 doc comment is incorrect. It allocates exactly the given size in bytes.
If we wanted it to allocate at most the given size in bytes, then new
should fall back to 1-TiB-and-go-down behavior when new_exact
returns None
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.
Ah, yeah, originally I had it step down from bytes.unwrap_or(1 << 40)
, but I thought it was undesirable if someone explicitly passed an amount of memory and it was silently downgraded.
Works fine, and blazing fast.
It's correct and I believe it covers all usecases we have right now. |
Heap::new_exact
, which takes a size in words, and is a useful low-level API for testing/programmatic contextsHeap::new
is useful for user-controlled heap allocations; it takes an optional size in bytes, and falls back to the try-1-TiB-and-go-down-from-there approach@FranchuFranchu can you confirm that running hvmc with no arguments works on your machine, still?
Also, @FranchuFranchu thoughts on this new API?