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.
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. |
Dentosal
added
breaking
| 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.
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.
Agreed. It's definitely not too easy to see that this cannot overflow.
Closes #452
Spec PR: FuelLabs/fuel-specs#566