Allow single opcode stepping for EVM #9051

sorpaas · 2018-07-05T12:39:14Z

This is a prerequisite for implementing EVM resume and callstack refactoring. In particular, a new function Interpreter::step(&mut Ext) is added. This function step one additional opcode, and can be used plainly. Other notable changes:

ActionParams is taken when initializing a VM, but not when calling exec. This is because we need to allocate stack, gasometer, etc before exec is called.
The ability to "reuse" a VM after an execution is removed. We never did that in our code. Allocation (either on stack or heap) for various VM structs need to happen anyway, so "reusing" doesn't provide performance benefits (except maybe reusing the previous VM's memory heap region).

sorpaas · 2018-07-10T08:42:45Z

Here's the benchmark results:

jsontests benchmarks:

{'ackermann31-INT': 0.9897959183673469,
 'ackermann32-INT': 0.9167478091528725,
 'ackermann33-INT': 0.9596136962247586,
 'fibonacci10-INT': 1.0860832137733143,
 'fibonacci16-INT': 0.9300284017557449,
 'loop-add-10M-INT': 0.9226587206771758,
 'loop-divadd-10M-INT': 0.9229079794970425,
 'loop-divadd-unr100-10M-INT': 0.8998631955175739,
 'loop-exp-16b-100k-INT': 1.0188090390141178,
 'loop-exp-1b-1M-INT': 0.9923591588497622,
 'loop-exp-2b-100k-INT': 0.9466506030511994,
 'loop-exp-32b-100k-INT': 1.0991487959483188,
 'loop-exp-4b-100k-INT': 1.12556930650952,
 'loop-exp-8b-100k-INT': 1.1088218849334053,
 'loop-exp-nop-1M-INT': 0.8219694300785856,
 'loop-mul-INT': 0.8684106995830124,
 'loop-mulmod-2M-INT': 0.9754786210274101,
 'manyFunctions100-INT': 0.8409570724841661}

Uint benchmark (master):

Output: 0x606060405200
Gas used: 29fb170
Time: 0.407322134s
^^^^ usize
Output: 0x606060405200
Gas used: 29fb170
Time: 0.418064895s
^^^^ U256
Output: 0x606060405200
Gas used: 8865053
Time: 1.412694181s
^^^^ usize
Output: 0x606060405200
Gas used: 8865053
Time: 1.473797155s
^^^^ U256

Uint benchmark (PR):

Output: 0x606060405200
Gas used: 29fb170
Time: 0.472320852s
^^^^ usize
Output: 0x606060405200
Gas used: 29fb170
Time: 0.489357480s
^^^^ U256
Output: 0x606060405200
Gas used: 8865053
Time: 1.608301138s
^^^^ usize
Output: 0x606060405200
Gas used: 8865053
Time: 1.618444041s
^^^^ U256

This gist contains the python code used to format the result.

tomusdrw

Looks really good, couple of minor grumbles.

tomusdrw · 2018-07-17T07:32:18Z

ethcore/evm/src/interpreter/mod.rs

+	/// Execute a single step on the VM.
+	#[inline(always)]
+	pub fn step(&mut self, ext: &mut vm::Ext) -> InterpreterResult {
+		macro_rules! try_or_done {


Is it really worth to have a macro for that?
Maybe sth like this:

#[inline(always)] fn try_or_done<F>(x: Result<F, vm::Error>) -> Result<F, InterpreterResult> { x.map_err(|e| InterpreterResult::Done(Err(e)) } // and used like this: try_or_done(self.verify_instruction(ext, instruction, info))?;

Or maybe even add From<vm::Error> for InterpreterResult and convert errors automatically via ??

The issue is that ? only works for Result like things with Ok and Err. So we can't use any ? syntax in step right now, unless we change InterpreterResult to be a Result<A, B>. This indeed can be done via:

Return Ok(result), which is the same case for InterpreterResult::Done(_).

Return error case for InterpreterResult::Stopped and InterpreterResult::Continue.

However, I think it may be a little bit confusing as Ok/Err do not really apply here.

Oh, that's true. Forgot that it only works for Result on the return type as well, anyway might be worth extracting the stuff in inner to a separate function and maybe use a helper function instead of a macro. What do you think?

I think I found a way: we can return Result<(), InterpreterResult> as the inner function and always return Err. The compiler would happily accept ? syntax with that. And we would just need to make sure the inner function never returns Ok(()).

This detail is wrapped in step_inner so it doesn't affect public interface.

tomusdrw · 2018-07-17T07:33:56Z

ethcore/evm/src/interpreter/mod.rs


-			let info = instruction.info();
-			self.verify_instruction(ext, instruction, info, &stack)?;
+				if instruction.is_none() {


let instruction = match instruction { Some(i) => i, None => return vm::Error::BadInstruction { .. }.into(); }

tomusdrw · 2018-07-17T07:39:37Z

ethcore/evm/src/interpreter/mod.rs

-			let instruction = Instruction::from_u8(opcode);
-			reader.position += 1;
+		let result = {
+			let mut inner = || {


Why do we need the inner closure?

Oh, I see to be able to use return maybe it's worth to move it to a separate function?

tomusdrw · 2018-07-17T07:42:18Z

ethcore/evm/src/interpreter/mod.rs

+		let result = {
+			let mut inner = || {
+				// This case is needed because code length can be zero.
+				if self.reader.position >= self.reader.len() {


Seems that we are checking that twice now, can't this be moved as some initial step? The reader.position is changed only during regular execution and jumps, right?

I moved it up to check just reader.len() == 0. That check still needs to happen every time step is called, otherwise we'll need an additional "interpreter state" (which might actually cost more memory).

tomusdrw · 2018-07-17T07:44:05Z

ethcore/evm/src/interpreter/mod.rs

 	}

-	fn verify_instruction(&self, ext: &vm::Ext, instruction: Instruction, info: &InstructionInfo, stack: &Stack<U256>) -> vm::Result<()> {
+	fn verify_instruction(&self, ext: &vm::Ext, instruction: Instruction, info: &InstructionInfo) -> vm::Result<()> {
 		let schedule = ext.schedule();


Schedule could probably be cached on self level, less virtual calls.

The issue is that for some use cases of step, like pausing, we would want to take back the Ext reference, possibly do some modifications to it, before passing it back to continue the execution.

If we take a reference of Schedule and put it on self level, it means that this single Ext is not usable again for the whole lifecycle of the Interpreter.

Or we need to clone the Schedule, which may also be expensive.

I think virtual calls might not be that expensive. And with (1), one optimization we can do is to split Ext up to a) interpreter-specific Ext, and b) transaction-level Ext. I think that may be a good trade-off compared with additional virtual calls during each step.

Makes sense. I thought that we already clone schedule on ext.schedule() but now saw it's only borrowed.

tomusdrw · 2018-07-17T07:45:33Z

ethcore/evm/src/interpreter/mod.rs

@@ -102,127 +103,234 @@ enum InstructionResult<Gas> {
 	StopExecution,
 }

+/// ActionParams without code, so that it can be feed into CodeReader.
+#[derive(Debug)]
+struct InterpreterParams {


Why we need this? Couldn't we just use ActionParams and add some info that the code is always None?

Actually on a second though, it's better to have it like that - prevents future errors of accessing the code.

…-vm-step

sorpaas · 2018-07-24T07:47:06Z

A new change on this PR is that I made Interpreter::new to always succeeds. We only had one possible Err case -- gasometer, if usize cost type is used, can directly return out of gas error. This is changed to delay the error return to step -- by making gasometer an option, and on step, if we found gasometer to be option, then directly return error.

The rationale for this is that I found out if Interpreter::new can return error, it would significantly complicate resumable Executive implementation.

…-vm-step

andresilva

LGTM

sorpaas added 12 commits July 5, 2018 18:18

Feed in ActionParams on VM creation

3850eae

Fix ethcore after Vm interface change

73f58ea

Move informant inside Interpreter struct

db4f030

Move do_trace to Interpreter struct

b3cbf24

Move all remaining exec variables to Interpreter struct

3145fc5

Refactor VM to allow single opcode step

b98129e

Fix all EVM tests

bb944c8

Fix all wasm tests

9ea1409

Fix wasm runner tests

c8a3a54

Fix a check case where code length is zero

5f0a419

Fix jsontests compile

e2bb546

Fix cargo lock

bb303d7

sorpaas added A0-pleasereview 🤓 Pull request needs code review. F6-refactor 📚 Code needs refactoring. M4-core ⛓ Core client code / Rust. labels Jul 5, 2018

sorpaas added this to the 1.12 milestone Jul 5, 2018

5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 10, 2018

5chdn modified the milestones: 2.0, 2.1 Jul 10, 2018

5chdn removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 11, 2018

5chdn requested review from tomusdrw and debris July 13, 2018 10:26

tomusdrw approved these changes Jul 17, 2018

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 17, 2018

sorpaas added 4 commits July 17, 2018 16:13

Use match instead of expect

8578b20

Use cheaper check reader.len() == 0 for the initial special case

9e8b23c

Get rid of try_and_done! macro by using Result<(), ReturnType>

e6fb25b

Use Never instead of ()

151101a

5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018

sorpaas added 5 commits July 24, 2018 15:23

Merge branch 'master' of https://github.com/paritytech/parity into sp…

ee4a476

…-vm-step

Fix parity-bytes path

1d1a319

Bypass gasometer lifetime problem by borrow only for a instance

45f715b

typo: missing {

21b8729

Fix ethcore test compile

104f844

sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 24, 2018

sorpaas added 2 commits July 24, 2018 16:39

Fix evm tests

f936a56

Merge branch 'master' of https://github.com/paritytech/parity into sp…

b165893

…-vm-step

sorpaas force-pushed the sp-vm-step branch from b09032e to b165893 Compare August 3, 2018 10:02

andresilva approved these changes Aug 13, 2018

View reviewed changes

sorpaas merged commit 9c595af into master Aug 13, 2018

sorpaas deleted the sp-vm-step branch August 13, 2018 20:06

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. F6-refactor 📚 Code needs refactoring. labels Aug 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow single opcode stepping for EVM #9051

Allow single opcode stepping for EVM #9051

sorpaas commented Jul 5, 2018 •

edited

Loading

sorpaas commented Jul 10, 2018

tomusdrw left a comment

tomusdrw Jul 17, 2018

sorpaas Jul 17, 2018

tomusdrw Jul 17, 2018

sorpaas Jul 17, 2018

tomusdrw Jul 17, 2018

tomusdrw Jul 17, 2018

tomusdrw Jul 17, 2018

sorpaas Jul 17, 2018

tomusdrw Jul 17, 2018

sorpaas Jul 17, 2018

tomusdrw Jul 17, 2018

tomusdrw Jul 17, 2018

tomusdrw Jul 17, 2018

sorpaas commented Jul 24, 2018 •

edited

Loading

andresilva left a comment

Allow single opcode stepping for EVM #9051

Allow single opcode stepping for EVM #9051

Conversation

sorpaas commented Jul 5, 2018 • edited Loading

sorpaas commented Jul 10, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas commented Jul 24, 2018 • edited Loading

andresilva left a comment

Choose a reason for hiding this comment

sorpaas commented Jul 5, 2018 •

edited

Loading

sorpaas commented Jul 24, 2018 •

edited

Loading