-
Notifications
You must be signed in to change notification settings - Fork 86
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
Store stack and heap separately #697
Conversation
fuel-vm/src/interpreter/memory.rs
Outdated
// It is how `Vec::resize` calculates a new capacity. | ||
let cap = core::cmp::max(len * 2, new_len); | ||
let cap = core::cmp::min(cap, MEM_SIZE); | ||
let diff = cap - len; | ||
let mut new_vec = vec![value; cap]; | ||
new_vec[diff..].copy_from_slice(self.as_slice()); | ||
*self = new_vec; |
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 at least just copies the bytes just once, but it also forces a new allocation every time. I guess that cannot be avoided if we want to use a single Vec
. We could maybe be a bit smarter than just doubling the size every time, e.g. starting with a kilobyte-sized allocation and not allowing the last doubling to MEM_SIZE
, instead allocating with reserve_exact
.
In any case, ALOC
is currently constant cost and will allow copying the amount of entire memory, in parts. What used to be a cheap subtract and compare operation, is now a memcpy.
There's a subtle bug in the implementation here: It's possible to extend the stack, then shrink the stack, and then extend the heap to be where the stack once was. Then reading from that memory area would read from the memory that was in the stack, and not from the heap that's on that region now. Fortunately we never shrink the heap, so just truncating the `stack´ field every time we grow heap over it fixes that. Fixed in 10fed83 |
Another breaking change: Allocating new heap space with ALOC now zeroes memory. |
try_update_stack_pointer(sp, ssp, hp, stack_range.words().end)?; | ||
let write_size = count * (core::mem::size_of::<Word>() as u64); | ||
let write_at = *sp; | ||
try_update_stack_pointer(sp, ssp, hp, write_at + write_size, memory)?; |
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 know that it shouldn't be possible to overflow here, but it would be nice to add #![deny(clippy::arithmetic_side_effects)]
for fuel-vm
. But it is definitely not a part of the current PR=)
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.
Agreed. It's definitely not too easy to see that this cannot overflow.
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 super good to me=) Approved! (I can't approve since I created the draft PR)
Closes #452
Spec PR: FuelLabs/fuel-specs#566