Refactor interpreter flow module #14
Conversation
If an attempt to increase the stack space results in memory intersection with the heap, then `ExecuteError::StackHeapCollision` should be returned Resolves #19
Instruction calls are to return `Result` instead of bool, except for ALU Memory ownership checks should be centralized in its module
vlopes11
force-pushed
the
vlopes11/flow-refactor
branch
from
bcfe406 to
eed8833
Compare
…nto vlopes11/flow-refactor
| self.registers[REG_SP] = result; | ||
| true | ||
| pub(crate) fn extend_call_frame(&mut self, imm: Word) -> Result<(), ExecuteError> { | ||
| if self.registers[REG_SP] > VM_MAX_RAM - imm { |
There was a problem hiding this comment.
What if imm is 2^24-1? That's 16 MB, but VM_MAX_RAM might be less than that. This question applies to other unsafe arithmetic as well.
There was a problem hiding this comment.
That is true, I was under the wrong assumption that the VM ram is 32M when it is 1.
However, this brings up a new question: how can the MEM_MAX_ACCESS_SIZE (currently 32M) be bigger than VM_MAX_RAM? Should I reduce the MEM_MAX_ACCESS_SIZE to VM_MAX_RAM?
https://github.com/FuelLabs/fuel-vm/blob/master/src/consts.rs#L9-L10
There was a problem hiding this comment.
It shouldn't be bigger, no. It should be reduced. But back to this specific issue: these subtractions are actually unsafe and shouldn't be optimized away. This PR should instead focus on refactoring the flow module to return result, and leave optimizations to future PRs if needed.
There was a problem hiding this comment.
Yes, that is correct. Without the assumption that VM_MAX_RAM == 32m, that calculation is unsafe.
That can be trivially solved by prepending one of these two:
imm > VM_MAX_RAM || self.registers[REG_SP] > VM_MAX_RAM - immimm > MEM_MAX_ACCESS_SIZE || self.registers[REG_SP] > VM_MAX_RAM - imm
I'll replicate this into the other verifications, ok?
There was a problem hiding this comment.
While those checks seem to solve the issue, I think it would be more idiomatic to use checked_sub, no?
There was a problem hiding this comment.
I don't know enough about the vm memory model to verify all this unsafe code, but I wonder do we really need all this performance right now? Shouldn't we focus on being as correct as possible first, benchmark, then optimize?
There was a problem hiding this comment.
Agree with @Voxelot. There will absolutely be time for optimizations later, but for now the priority should be functionality and correctness.
There was a problem hiding this comment.
@Voxelot the $sp can't ever be 0 or smaller than $ssp/$fp; otherwise its a severe violation of the initialization routine. There is a nit in the example code but it applies to both functions (if_add and checked_add): it should be >= VM_MAX_RAM, and not >.
As for the approach, all clear, but this is not hard optimization. My goal for now is to achieve a final form of the API, and these changes reflect on how the data backend should interact with the VM (e.g. last change to use references in the data trait).
If we achieve the final form of the API, then any further optimization will be done internally and/or incrementally.
A next step is to add some serious test coverage to catch all little edge cases.
There was a problem hiding this comment.
Given these are just opcode implementation details I don't see how these relate to holding up the design of the public api interface? This seems purely like premature optimization to me.
There was a problem hiding this comment.
We will know if these optimizations won't reflect anything into the API/sway/client/core after we have the full integration tests done.
Also, if you check, the signature of the functions changed. Instead of returning bool, they are returning Result. As much as that is not expected to impact sway/client, it is important to achieve consistency with these functions internally.
When I'm done migrating all of them to results (including the other PRs of the other modules), I'll then update the execute function. In that function, I'm (for the moment) using is_ok while I don't have all of them migrated
| self.registers[ra] = self.block_height() as Word | ||
| Opcode::BHEI(ra) if Self::is_register_writable(ra) && self.gas_charge(&op).is_ok() => { | ||
| self.registers[ra] = self.block_height() as Word; | ||
| self.inc_pc(); |
There was a problem hiding this comment.
These calls to inc_pc seems redundant and inconsistent. In some cases they are in the root opcode impl, and sometimes performed in lower methods.
Couldn't we just check the result of the opcode match and then do inc_pc in a single location?
There was a problem hiding this comment.
These refactorings aim (also) to move all of them into the functions. Some opcodes increment the PC while some others doesn't
There is a lot of small nits like that I'm fixing with these PRs. When I'm finished with them, I'll change the way execute is implemented and create the constants for the gas cost
|
N/A |
* Add script data offset function to tx Script data offset will need to be accessible to users of the transaction API in order to create data for scripts * Update transaction script offset to return Option The API must be consistent and when a function that is directed to a specifc variant of transaction is called over a script or vice-versa, it should be protected by either a result or an option.
* Add serde-types feature * Add iterator to bytes capability for Opcode
* Reintroduce auto_impl * cargo sort
* Reintroduce auto_impl * cargo sort
* Add script data offset function to tx Script data offset will need to be accessible to users of the transaction API in order to create data for scripts * Update transaction script offset to return Option The API must be consistent and when a function that is directed to a specifc variant of transaction is called over a script or vice-versa, it should be protected by either a result or an option.
* Add serde-types feature * Add iterator to bytes capability for Opcode
* Reintroduce auto_impl * cargo sort
* Add script data offset function to tx Script data offset will need to be accessible to users of the transaction API in order to create data for scripts * Update transaction script offset to return Option The API must be consistent and when a function that is directed to a specifc variant of transaction is called over a script or vice-versa, it should be protected by either a result or an option.
* Add serde-types feature * Add iterator to bytes capability for Opcode
* Reintroduce auto_impl * cargo sort
* Add script data offset function to tx Script data offset will need to be accessible to users of the transaction API in order to create data for scripts * Update transaction script offset to return Option The API must be consistent and when a function that is directed to a specifc variant of transaction is called over a script or vice-versa, it should be protected by either a result or an option.
* Add serde-types feature * Add iterator to bytes capability for Opcode
Instruction calls are to return
Resultinstead of bool, except for ALUMemory ownership checks should be centralized in its module