Paged memory allocation #461
Conversation
fuel-vm/src/interpreter/balances.rs
Outdated
| @@ -185,6 +184,7 @@ fn writes_to_memory_correctly() { | |||
|
|
|||
| let rng = &mut StdRng::seed_from_u64(2322u64); | |||
| let mut interpreter = Interpreter::<_, Script>::without_storage(); | |||
| interpreter.reset(); | |||
There was a problem hiding this comment.
Why do we need to reset here?
| self.memory[memory_offset..memory_offset_end] | ||
| .iter_mut() | ||
| .for_each(|m| *m = 0); | ||
| self.memory.force_clear(MemoryRange::try_new(memory_offset, length)?); |
There was a problem hiding this comment.
Hmm, it clears the stack and the heap, but we only want to clear the stack(based on the comments above).
| @@ -338,13 +353,8 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { | |||
|
|
|||
| let code = &contract[..len]; | |||
There was a problem hiding this comment.
The comment is related to the code above:
let contract = super::contract::contract(self.storage, contract_id)?;
Hmm, it s seems we need to use API created by the Tommi and load the code directly into the memory.
But to do so, we need to allocate the memory in the stack first.
It is not related to this PR, we need to update it in the follow-up
| // perform the code copy | ||
| memory.copy_from_slice(code); | ||
| self.memory.force_write_slice(memory_offset, code); |
There was a problem hiding this comment.
Hmm, how do we know that the memory is allocated here?
| // Safe to cast as we've shifted the 8 MSB. | ||
| let reason_u8 = (val >> REASON_OFFSET) as u8; | ||
| // Cast to truncate in order to remove the `reason` bits. | ||
| let instruction = (val >> INSTR_OFFSET) as u32; | ||
| let reason = PanicReason::try_from(reason_u8)?; | ||
| Ok(Self { reason, instruction }) | ||
| let reason = PanicReason::from(reason_u8); |
There was a problem hiding this comment.
Why do we want to change it in this PR?
I'm okay with treating all unknown numbers as unknown, but the idea was don't allow panic with a value of zero because it means something is wrong in our internal logic=) And my question is do we need it in this PR or can we do that in another?
| @@ -55,6 +55,15 @@ impl Call { | |||
| pub const fn into_inner(self) -> (ContractId, Word, Word) { | |||
| (self.to, self.a, self.b) | |||
| } | |||
|
|
|||
| /// Restore a call structure from a byte array. | |||
| pub fn from_bytes(bytes: [u8; Self::LEN]) -> Self { | |||
There was a problem hiding this comment.
Hmm, it seems you don't use this function, maybe you want do reuse it in the from
| /// Context, i.e. current contract | ||
| pub const fn context(&self) -> Option<ContractId> { | ||
| self.context | ||
| } | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| /// Offset from the IS register | ||
| pub const fn offset(&self) -> u64 { | ||
| self.offset |
There was a problem hiding this comment.
Fields are public, so it doesn't make sense to have getters=)
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| &mut self.0 | ||
| impl<S, Tx> Interpreter<S, Tx> { | ||
| pub(crate) fn current_location(&self) -> InstructionLocation { |
There was a problem hiding this comment.
Previously it was under #[cfg(feature = "profile-gas")] flag and didn't participate in the production. But now, it will. I think we need to put fields under this flag too(or use different InstructionLocation for each flag)
| }, | ||
| mut w, | ||
| ) = split_registers(&mut self.registers); | ||
| let dest = &mut w[ra.try_into()?]; |
There was a problem hiding this comment.
Hmm, I don't know how it worked before, but for me w[ra](where w is a writable registers without system registers) and self.registers[ra](it is system registers and writable together) are different things. Because the lengths of arrays are different and with the same ra, they will point o different registers.
Is that a bug? If yes, what is the right version?
| } else { | ||
| c.into() | ||
| }; | ||
|
|
||
| *dest = [<cmp_ $t:lower>](lhs, rhs, args.mode); |
There was a problem hiding this comment.
The same here. What is the right variant?
| @@ -91,14 +92,12 @@ impl RuntimeBalances { | |||
| self.state.get(asset).map(Balance::value) | |||
| } | |||
|
|
|||
| fn set_memory_balance_inner(balance: &Balance, memory: &mut [u8; MEM_SIZE]) -> Result<Word, RuntimeError> { | |||
| fn set_memory_balance_inner(balance: &Balance, memory: &mut VmMemory) -> Result<Word, RuntimeError> { | |||
There was a problem hiding this comment.
If we are sure that write_bytes can't fail, then let's change the signature of the method too
| fn set_memory_balance_inner(balance: &Balance, memory: &mut VmMemory) -> Result<Word, RuntimeError> { | |
| fn set_memory_balance_inner(balance: &Balance, memory: &mut VmMemory) -> Word { |
| return Err(PanicReason::MemoryAccessSize.into()); | ||
| } | ||
|
|
||
| if stack.contains_range(range) && heap.contains_range(range) { |
There was a problem hiding this comment.
How range can belong to the heap and to the stack simultaneously?
| } => matches Ok(Output { external_balance: 20, internal_balance: 0, .. }) | ||
| ; "spend all coins succesfully from internal context" | ||
| )] | ||
| fn test_smo( |
There was a problem hiding this comment.
why was this test removed? It has little to do with these memory changes and is more about verifying how smo affects runtime balances vs contract balances.
Dentosal
force-pushed
the
dento/paged-memory
branch
from
6101ed0
to
0d4efc3
Compare
|
Found a bug related to the issue I anticipated with this approach before we started implementing this. Because new pages are appended to the end of the heap buffer, the memory address translation isn't stable. I wrote a test which writes some data to the heap, logs it, allocates a new page, then tries to log the same data again. On master this test passes, but on this branch it fails. This is because after the heap buffer is extended, the Opened a PR with the test here -> #468 |
|
Nevermind |
Closes #452. Spec PR FuelLabs/fuel-specs#489.
Still has lots of unimplemented details.