EVM: Further dispatch refactoring #876
Conversation
it's just one, the intrinsic, which passes a machine.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #876 +/- ##
==========================================
+ Coverage 87.10% 87.12% +0.02%
==========================================
Files 126 126
Lines 23278 23346 +68
==========================================
+ Hits 20276 20341 +65
- Misses 3002 3005 +3
|
| match instructions::$ins($m) { | ||
| Ok($res) => $body, | ||
| Err(err) => { | ||
| $m.exit = Some(err); | ||
| } |
There was a problem hiding this comment.
I'd be interested to see if the following were faster:
$m.exit = instructions::$ins($m).err()
$m.pc += 1; // when actually changing the PC.In all cases. Basically, avoid the branches.
Of course, that might actually be slower in terms of gas used. But I'm still curious.
There was a problem hiding this comment.
err branches on its own perhaps? Or is it a vtable thing?
I am skeptical whether this would be any faster, and it is also somewhat harder to reason about.
There was a problem hiding this comment.
also, there is an assignment that doesnt need to happen 99.9% of the time; potentially just as bas as a correctly predicted branch or worse.
| self.step(); | ||
|
|
||
| if self.exit.is_some() { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Hm. Can we move this condition to the top to reduce the number of breaks?
I.e.:
while self.exit.is_none() && self.pc < self.bytecode.len() {
self.step();
}There was a problem hiding this comment.
ok, somewhat prettier; my way is more explicit however
There was a problem hiding this comment.
Some more thoughts here: the current code is optimal in terms of branches.
Initially, exit is known to be None (initial state invariant), but the code may be empty so we need to check pc against the bound.
Then we execute the instruction, which may change exit, at which point we check it and loop.
No matter how you look at it, the prettier variant will execute one more check/possible branch than the current code.
So i suggest we keep it as is.
| let op = OpCode::try_from(self.bytecode[self.pc]); | ||
| match op { | ||
| Ok(op) => Self::JMPTABLE[op as usize](self), | ||
| Err(err) => self.exit = Some(err), | ||
| } |
There was a problem hiding this comment.
Instead of checking if PC is "in bounds above and inside step, we can just check inside step:
| let op = OpCode::try_from(self.bytecode[self.pc]); | |
| match op { | |
| Ok(op) => Self::JMPTABLE[op as usize](self), | |
| Err(err) => self.exit = Some(err), | |
| } | |
| match self.bytecode.get(self.pc).ok_or(StatusCode::Success).map(OpCode::try_from) { | |
| Ok(op) => Self::JMPTABLE[op as usize](self), | |
| Err(err) => self.exit = Some(err), | |
| } |
Then remove the self.pc < self.bytecode.len() check above.
There was a problem hiding this comment.
We can probably also use unsafe when accessing the jumptable. That I'm happy with given that we know it's safe.
| m.reverted = true; | ||
| m.exit = Some(StatusCode::Success); |
There was a problem hiding this comment.
future: maybe StatusCode::Reverted?
There was a problem hiding this comment.
If we have multiple ways to revert, yeah.
| @@ -137,36 +134,90 @@ mod test { | |||
| } | |||
| } | |||
|
|
|||
| pub fn ins_add(s: &mut Stack) { | |||
There was a problem hiding this comment.
nit: we should probably just fix the tests to call the instructions directly. We only tested the instructions with a proper stack because we had no other way.
There was a problem hiding this comment.
yeah, a chore for when we have nothing better to do.
Ever so slightly faster (0.5% improvement over next head in simplecoin), but much nicer code.
And if we turn off overflow checks, it's a 3.24% improvement.