-
Notifications
You must be signed in to change notification settings - Fork 84
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
Memory pool/reuse #732
Memory pool/reuse #732
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.
We also need to add a new constructor to the Interpreter
that accepts the memory as input and calls reset
on it during initialization.
#[cfg(test)] | ||
impl<Tx, Ecal> Interpreter<(), Tx, Ecal> | ||
#[cfg(any(test, feature = "test-helpers"))] | ||
impl<Tx, Ecal> Interpreter<MemoryFromPool, (), Tx, Ecal> |
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.
All functions below can just be generic
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.
Doesn't make much sense, when Default
already returns a concrete type.
@@ -29,19 +30,21 @@ where | |||
const INC_PC: bool = true; | |||
|
|||
/// ECAL opcode handler | |||
fn ecal<S, Tx>( | |||
vm: &mut Interpreter<S, Tx, Self>, | |||
fn ecal<M, S, Tx>( |
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.
Hmm, I think M
, S
, and Tx
should be generics of the EcalHandler
. Otherwise, the implementer doesn't have control over the constraints for these generics. The M: AsRef<Memory> + AsMut<Memory>
is an example. What is he needs some contains over the storage?
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.
Leaving this as a follow-up. #734
Renamed `Memory` into `MemoryInstance`.
# Conflicts: # fuel-vm/src/interpreter/blockchain.rs # fuel-vm/src/interpreter/flow.rs # fuel-vm/src/interpreter/flow/tests.rs
Used by #732
Adds a method to reset memory to the original state.
Also adds a memory pool to allow reusing memory instances, and use that for all VM initializations.
Checklist
Before requesting review