Cleanup, move hot fields toggether in Interpreter (#321)

bins/revm-test/src/bin/snailtracer.rs

@@ -1,7 +1,7 @@
 use std::time::Duration;
 
 use bytes::Bytes;
-use revm::{db::BenchmarkDB, Bytecode, CallContext, TransactTo, B160, U256};
+use revm::{db::BenchmarkDB, Bytecode, TransactTo};
 
 use revm_interpreter::{specification::BerlinSpec, DummyHost};
 extern crate alloc;
@@ -11,7 +11,7 @@ pub fn simple_example() {
 
     // BenchmarkDB is dummy state that implements Database trait.
     let mut evm = revm::new();
-    let bytecode = Bytecode::new_raw(contract_data);
+    let bytecode = Bytecode::new_raw(contract_data).to_analysed::<BerlinSpec>();
     evm.database(BenchmarkDB::new_bytecode(bytecode.clone()));
 
     // execution globals block hash/gas_limit/coinbase/timestamp..
@@ -38,23 +38,15 @@ pub fn simple_example() {
     );
 
     // revm interpreter
-
-    let contract = revm_interpreter::Contract::new_with_context::<BerlinSpec>(
-        evm.env.tx.data,
-        bytecode,
-        &CallContext {
-            address: B160::zero(),
-            caller: evm.env.tx.caller,
-            code_address: B160::zero(),
-            apparent_value: U256::from(0),
-            scheme: revm::CallScheme::Call,
-        },
-    );
+    let contract = revm_interpreter::Contract {
+        input: evm.env.tx.data,
+        bytecode: bytecode.lock_analysed().unwrap(),
+        ..Default::default()
+    };
 
     let mut host = DummyHost::new(env);
     microbench::bench(&bench_options, "Snailtracer Interpreter benchmark", || {
-        let mut interpreter =
-            revm_interpreter::Interpreter::new::<BerlinSpec>(contract.clone(), u64::MAX, false);
+        let mut interpreter = revm_interpreter::Interpreter::new(contract.clone(), u64::MAX, false);
         interpreter.run::<_, BerlinSpec>(&mut host);
         host.clear()
     });

crates/interpreter/src/gas.rs

@@ -5,11 +5,16 @@ pub use calc::*;
 pub use constants::*;
 #[derive(Clone, Copy, Debug)]
 pub struct Gas {
+    /// Gas Limit
     limit: u64,
+    /// used+memory gas.
+    all_used_gas: u64,
+    /// Used gas without memory
     used: u64,
+    /// Used gas for memory expansion
     memory: u64,
+    /// Refunded gas. This gas is used only at the end of execution.
     refunded: i64,
-    all_used_gas: u64,
 }
 impl Gas {
     pub fn new(limit: u64) -> Self {
@@ -51,7 +56,7 @@ impl Gas {
         self.refunded += refund;
     }
 
-    /// Record an explict cost.
+    /// Record an explicit cost.
     #[inline(always)]
     pub fn record_cost(&mut self, cost: u64) -> bool {
         let (all_used_gas, overflow) = self.all_used_gas.overflowing_add(cost);
@@ -64,7 +69,7 @@ impl Gas {
         true
     }
 
-    /// used in memory_resize! macro
+    /// used in memory_resize! macro to record gas used for memory expansion.
     pub fn record_memory(&mut self, gas_memory: u64) -> bool {
         if gas_memory > self.memory {
             let (all_used_gas, overflow) = self.used.overflowing_add(gas_memory);
@@ -77,7 +82,8 @@ impl Gas {
         true
     }
 
-    /// used in gas_refund! macro
+    /// used in gas_refund! macro to record refund value.
+    /// Refund can be negative but self.refunded is always positive.
     pub fn gas_refund(&mut self, refund: i64) {
         self.refunded += refund;
     }

crates/interpreter/src/interpreter.rs

@@ -19,35 +19,37 @@ pub const STACK_LIMIT: u64 = 1024;
 pub const CALL_STACK_LIMIT: u64 = 1024;
 
 pub struct Interpreter {
-    /// Contract information and invoking data
-    pub contract: Contract,
     /// Instruction pointer.
     pub instruction_pointer: *const u8,
+    /// Return is main control flag, it tell us if we should continue interpreter or break from it
+    pub instruction_result: Return,
+    /// left gas. Memory gas can be found in Memory field.
+    pub gas: Gas,
     /// Memory.
     pub memory: Memory,
     /// Stack.
     pub stack: Stack,
-    /// left gas. Memory gas can be found in Memory field.
-    pub gas: Gas,
     /// After call returns, its return data is saved here.
     pub return_data_buffer: Bytes,
     /// Return value.
     pub return_range: Range<usize>,
-    /// Return is main control flag, it tell us if we should continue interpreter or break from it
-    pub instruction_result: Return,
     /// Is interpreter call static.
     pub is_static: bool,
+    /// Contract information and invoking data
+    pub contract: Contract,
     /// Memory limit. See [`crate::CfgEnv`].
     #[cfg(feature = "memory_limit")]
     pub memory_limit: u64,
 }
 
 impl Interpreter {
+    /// Current opcode
     pub fn current_opcode(&self) -> u8 {
         unsafe { *self.instruction_pointer }
     }
 
-    pub fn new<SPEC: Spec>(contract: Contract, gas_limit: u64, is_static: bool) -> Self {
+    /// Create new interpreter
+    pub fn new(contract: Contract, gas_limit: u64, is_static: bool) -> Self {
         #[cfg(not(feature = "memory_limit"))]
         {
             Self {
@@ -65,12 +67,12 @@ impl Interpreter {
 
         #[cfg(feature = "memory_limit")]
         {
-            Self::new_with_memory_limit::<SPEC>(contract, gas_limit, is_static, u64::MAX)
+            Self::new_with_memory_limit(contract, gas_limit, is_static, u64::MAX)
         }
     }
 
     #[cfg(feature = "memory_limit")]
-    pub fn new_with_memory_limit<SPEC: Spec>(
+    pub fn new_with_memory_limit(
         contract: Contract,
         gas_limit: u64,
         is_static: bool,
@@ -98,7 +100,12 @@ impl Interpreter {
         &self.gas
     }
 
-    /// Reference of interp stack.
+    /// Reference of interpreter memory.
+    pub fn memory(&self) -> &Memory {
+        &self.memory
+    }
+
+    /// Reference of interpreter stack.
     pub fn stack(&self) -> &Stack {
         &self.stack
     }
@@ -123,20 +130,26 @@ impl Interpreter {
         }
     }
 
+    /// Execute next instruction
+    #[inline(always)]
+    pub fn step<H: Host, SPEC: Spec>(&mut self, host: &mut H) {
+        // step.
+        let opcode = unsafe { *self.instruction_pointer };
+        // Safety: In analysis we are doing padding of bytecode so that we are sure that last
+        // byte instruction is STOP so we are safe to just increment program_counter bcs on last instruction
+        // it will do noop and just stop execution of this contract
+        self.instruction_pointer = unsafe { self.instruction_pointer.offset(1) };
+        eval::<H, SPEC>(opcode, self, host);
+    }
+
     /// loop steps until we are finished with execution
     pub fn run<H: Host, SPEC: Spec>(&mut self, host: &mut H) -> Return {
         // add first gas_block
         if USE_GAS && !self.gas.record_cost(self.contract.first_gas_block()) {
             return Return::OutOfGas;
         }
         while self.instruction_result == Return::Continue {
-            // step.
-            let opcode = unsafe { *self.instruction_pointer };
-            // Safety: In analysis we are doing padding of bytecode so that we are sure that last
-            // byte instruction is STOP so we are safe to just increment program_counter bcs on last instruction
-            // it will do noop and just stop execution of this contract
-            self.instruction_pointer = unsafe { self.instruction_pointer.offset(1) };
-            eval::<H, SPEC>(opcode, self, host);
+            self.step::<H, SPEC>(host)
         }
         self.instruction_result
     }
@@ -153,12 +166,7 @@ impl Interpreter {
             if ret != Return::Continue {
                 return ret;
             }
-            let opcode = unsafe { *self.instruction_pointer };
-            // Safety: In analysis we are doing padding of bytecode so that we are sure that last.
-            // byte instruction is STOP so we are safe to just increment program_counter bcs on last instruction
-            // it will do noop and just stop execution of this contract
-            self.instruction_pointer = unsafe { self.instruction_pointer.offset(1) };
-            eval::<H, SPEC>(opcode, self, host);
+            self.step::<H, SPEC>(host);
 
             // step ends
             let ret = host.step_end(self, self.is_static, self.instruction_result);
@@ -169,7 +177,7 @@ impl Interpreter {
         self.instruction_result
     }
 
-    /// Copy and get the return value of the interp, if any.
+    /// Copy and get the return value of the interpreter, if any.
     pub fn return_value(&self) -> Bytes {
         // if start is usize max it means that our return len is zero and we need to return empty
         if self.return_range.start == usize::MAX {

crates/interpreter/src/interpreter/bytecode.rs

@@ -175,24 +175,24 @@ impl Bytecode {
         }
     }
 
-    pub fn lock<SPEC: Spec>(self) -> BytecodeLocked {
-        let Bytecode {
-            bytecode,
-            hash,
-            state,
-        } = self.to_analysed::<SPEC>();
-        if let BytecodeState::Analysed { len, jumptable } = state {
-            BytecodeLocked {
-                bytecode,
+    pub fn lock_analysed(self) -> Option<BytecodeLocked> {
+        if let BytecodeState::Analysed { len, jumptable } = self.state {
+            Some(BytecodeLocked {
+                bytecode: self.bytecode,
                 len,
-                hash,
+                hash: self.hash,
                 jumptable,
-            }
+            })
         } else {
-            unreachable!("to_analysed transforms state to analysed");
+            None
         }
     }
 
+    pub fn lock<SPEC: Spec>(self) -> BytecodeLocked {
+        let bytecode = self.to_analysed::<SPEC>();
+        bytecode.lock_analysed().expect("We have analysed bytecode")
+    }
+
     /// Analyze bytecode to get jumptable and gas blocks.
     fn analyze<SPEC: Spec>(code: &[u8]) -> ValidJumpAddress {
         let opcode_gas = spec_opcode_gas(SPEC::SPEC_ID);
@@ -270,6 +270,14 @@ pub struct BytecodeLocked {
     jumptable: ValidJumpAddress,
 }
 
+impl Default for BytecodeLocked {
+    fn default() -> Self {
+        Bytecode::default()
+            .lock_analysed()
+            .expect("Bytecode default is analysed code")
+    }
+}
+
 impl BytecodeLocked {
     pub fn as_ptr(&self) -> *const u8 {
         self.bytecode.as_ptr()

crates/interpreter/src/interpreter/contract.rs

@@ -3,7 +3,7 @@ use crate::{alloc::vec::Vec, CallContext, Spec, B160, U256};
 use bytes::Bytes;
 use std::sync::Arc;
 
-#[derive(Clone)]
+#[derive(Clone, Default)]
 pub struct Contract {
     /// Contracts data
     pub input: Bytes,

crates/revm/src/evm_impl.rs

@@ -490,15 +490,15 @@ impl<'a, GSPEC: Spec, DB: Database, const INSPECT: bool> EVMImpl<'a, GSPEC, DB,
         );
 
         #[cfg(feature = "memory_limit")]
-        let mut interpreter = Interpreter::new_with_memory_limit::<GSPEC>(
+        let mut interpreter = Interpreter::new_with_memory_limit(
             contract,
             gas.limit(),
             false,
             self.data.env.cfg.memory_limit,
         );
 
         #[cfg(not(feature = "memory_limit"))]
-        let mut interpreter = Interpreter::new::<GSPEC>(contract, gas.limit(), false);
+        let mut interpreter = Interpreter::new(contract, gas.limit(), false);
 
         if INSPECT {
             self.inspector
@@ -546,7 +546,7 @@ impl<'a, GSPEC: Spec, DB: Database, const INSPECT: bool> EVMImpl<'a, GSPEC, DB,
                 }
                 // if we have enought gas
                 self.data.journaled_state.checkpoint_commit();
-                // Do analasis of bytecode streight away.
+                // Do analysis of bytecode streight away.
                 let bytecode = match self.data.env.cfg.perf_analyse_created_bytecodes {
                     AnalysisKind::Raw => Bytecode::new_raw(bytes.clone()),
                     AnalysisKind::Check => Bytecode::new_raw(bytes.clone()).to_checked(),
@@ -692,16 +692,15 @@ impl<'a, GSPEC: Spec, DB: Database, const INSPECT: bool> EVMImpl<'a, GSPEC, DB,
             );
 
             #[cfg(feature = "memory_limit")]
-            let mut interpreter = Interpreter::new_with_memory_limit::<GSPEC>(
+            let mut interpreter = Interpreter::new_with_memory_limit(
                 contract,
                 gas.limit(),
                 inputs.is_static,
                 self.data.env.cfg.memory_limit,
             );
 
             #[cfg(not(feature = "memory_limit"))]
-            let mut interpreter =
-                Interpreter::new::<GSPEC>(contract, gas.limit(), inputs.is_static);
+            let mut interpreter = Interpreter::new(contract, gas.limit(), inputs.is_static);
 
             if INSPECT {
                 // create is always no static call.