perf: refactor interpreter internals (take 2) #582

DaniPopes · 2023-08-03T16:49:40Z

Split off from (and supersedes) #558, see that PR for benches and list of changes.

Best reviewed on a commit-by-commit basis.

Results

Cachegrind

download cachegrind.tar.gz, extracts into crates/cachegrind
run:

cargo b -r -p revm-cachegrind --bin snailtracer
valgrind --tool=cachegrind target/release/snailtracer

Commit	Snailtracer (I)	Improvement
`449d6b9` (main)	505,445,058	N/A
`b3440b1`	441,772,332	-12.6%

crates/interpreter/src/instructions/host_env.rs

crates/interpreter/src/instructions/mod.rs

DaniPopes · 2023-08-22T18:39:08Z

Marking as draft until the other PRs are merged, so I can then rebase on top of main.

The changes in PR will only be in interpreter/src/{instructions/*,interpreter/*}.

The compiler generates much more favorable assembly if all the functions are casted to a `fn(_, _)` pointer before calling them. See <bluealloy#310 (comment)> for more information.

DaniPopes

Added descriptions of changes. Other minor (non functional) changes include:

pub -> pub(super) to denote we are not exporting these instructions individually
remove last argument of as_usize_or_fail! macro calls when it's InvalidOperandOOG, since it's the default

DaniPopes · 2023-08-25T18:43:10Z

crates/interpreter/src/instructions/stack.rs

changed stack methods return type from Option<IR> to Result<(), IR>

DaniPopes · 2023-08-25T18:45:17Z

crates/interpreter/src/instructions/system.rs

use U256 and get_unchecked in calldataload

DaniPopes · 2023-08-25T18:51:34Z

crates/interpreter/src/interpreter.rs

dedup code in initializer functions

changed return range to (return length, return offset). Makes parsing the code easier. Before we were MAX..MAX to denote a length of zero, when we can just set the length and offset directly

added return_value_slice

DaniPopes · 2023-08-25T18:57:35Z

crates/interpreter/src/interpreter/stack.rs

Display should not be gated to std

avoid using manual implementations (like swap and pop) where possible by deferring to the standard library. This produces the same instructions while being safer

changed stack methods return type from Option<IR> to Result<(), IR>. This is clearer since Some would be the error

dedup some code by reusing the other functions (like pop_top -> pop + top)

DaniPopes · 2023-08-25T19:01:12Z

crates/interpreter/src/interpreter/memory.rs

changed runtime panics to be unreachable_unchecked in release mode since they should never happen. This helps performance quite a bit

DaniPopes · 2023-08-25T19:07:04Z

crates/interpreter/src/instructions/control.rs

factored out the set up in common for return and revert

DaniPopes · 2023-08-25T19:07:22Z

crates/interpreter/src/instructions/bitwise.rs

cmp opcodes are now branchless

byte indexes into the slice instead of performing shifts

sar uses U256::MAX instead of computing it at runtime

DaniPopes · 2023-08-25T19:07:34Z

crates/interpreter/src/instructions/arithmetic.rs

avoid reassigning to 0 when already 0 in div and rem

DaniPopes · 2023-08-25T19:07:46Z

crates/interpreter/src/instructions.rs

moved to eval to opcode.rs, and the return instructions to control

DaniPopes · 2023-08-25T19:10:27Z

crates/interpreter/src/instructions/opcode.rs

+};
+use core::fmt;
+
+macro_rules! opcodes {


This macro declares all of:

const NAME: u8 = $value;

OPCODE_JUMPMAP[$value] = Some(stringify!($name));

eval dispatch

Left out gas, which is improved below.

was wondering if gas should be part of the opcode, but maybe not? it'd be nice if they were all declared in one place, but ig fine as-is?

DaniPopes · 2023-08-25T19:10:46Z

crates/interpreter/src/instructions/opcode.rs

+// When adding new opcodes:
+// 1. add the opcode to the list below; make sure it's sorted by opcode value
+// 2. add its gas info in the `opcode_gas_info` function below
+// 3. implement the opcode in the corresponding module;
+//    the function signature must be the exact same as the others


Added instructions on how to add new opcodes with the new macro.

DaniPopes · 2023-08-25T19:11:46Z

crates/interpreter/src/instructions/opcode.rs

+            write!(f, "UNKNOWN(0x{n:02X})")
+        }
+    }
+}

 impl OpCode {


Deprecated:

try_from_u8 -> new

u8 -> get

This is the same naming scheme as NonZero* std numbers.

DaniPopes · 2023-08-25T19:12:31Z

crates/interpreter/src/instructions/opcode.rs

-            /* 0xff  SELFDESTRUCT */ OpInfo::gas_block_end(0),
-        ];
-    };
+const fn opcode_gas_info(opcode: u8, spec: SpecId) -> OpInfo {


moved gas declarations from macros to const fns.

Is this something you'd want to move on the main OPcode macro? i.e make it point to a statement that returns OpInfo?

I considered doing this but each match arm is pretty substantial for each opcode and would clutter the main invocation

gakonst

love the explanations on each change. very helpful.

generally supportive - this seems hard to break down further (except the visibility changes I guess). i like the new opcode declaration method. seems like const fns are paying off.

the changes outside of opcode.rs seem non-controversial/pure improvements, but defer to Dragan for all that

gakonst · 2023-08-25T19:23:32Z

crates/interpreter/src/instructions/opcode.rs

-            /* 0xff  SELFDESTRUCT */ OpInfo::gas_block_end(0),
-        ];
-    };
+const fn opcode_gas_info(opcode: u8, spec: SpecId) -> OpInfo {


Is this something you'd want to move on the main OPcode macro? i.e make it point to a statement that returns OpInfo?

gakonst · 2023-08-25T19:27:28Z

crates/interpreter/src/instructions/opcode.rs

+};
+use core::fmt;
+
+macro_rules! opcodes {


was wondering if gas should be part of the opcode, but maybe not? it'd be nice if they were all declared in one place, but ig fine as-is?

rakita · 2023-09-19T14:21:51Z

crates/interpreter/src/instructions/opcode.rs

+            // for previous efforts on optimizing this function.
+            let f: Instruction = match opcode {
+                $($name => $f as Instruction,)*
+                _ => control::not_found as Instruction,


I like this a lot! casting to Instruction allows us to be sure that every fn has the same signature. @DaniPopes do you think this is the reason for the performance boost?

IIRC it's a bit more than half of the total perf improvement of this pr

rakita · 2023-09-20T10:59:17Z

@DaniPopes added mostly nits, moved all instructions to be public as that can be useful, and added some small changes.

very nice PR!

rakita

the test is failing for an unrelated reason.

DaniPopes mentioned this pull request Aug 3, 2023

perf: refactor interpreter internals #558

Closed

DaniPopes commented Aug 3, 2023

View reviewed changes

crates/interpreter/src/instructions/host_env.rs Show resolved Hide resolved

DaniPopes force-pushed the refactor-interpreter branch 2 times, most recently from b92f2c3 to 967ac6c Compare August 3, 2023 17:37

DaniPopes mentioned this pull request Aug 3, 2023

New dispatch strategy: Add array of instruction handles #310

Closed

DaniPopes force-pushed the refactor-interpreter branch from 967ac6c to 4f6452a Compare August 3, 2023 20:40

rakita reviewed Aug 4, 2023

View reviewed changes

crates/interpreter/src/instructions/mod.rs Outdated Show resolved Hide resolved

DaniPopes force-pushed the refactor-interpreter branch 6 times, most recently from 1e8227d to c49fe6d Compare August 9, 2023 16:14

DaniPopes force-pushed the refactor-interpreter branch 5 times, most recently from eddc2af to b3440b1 Compare August 16, 2023 16:51

DaniPopes force-pushed the refactor-interpreter branch from b3440b1 to 162a57b Compare August 22, 2023 08:22

This was referenced Aug 22, 2023

chore(interpreter): use let else #629

Merged

perf(interpreter): improve i256 instructions #630

Merged

chore(interpreter): improve dummy host #631

Merged

chore(interpreter): improve gas calculations #632

Merged

chore: misc improvements #633

Merged

DaniPopes marked this pull request as draft August 22, 2023 18:39

DaniPopes force-pushed the refactor-interpreter branch 2 times, most recently from f3f046d to 924e959 Compare August 24, 2023 14:07

DaniPopes added 2 commits August 25, 2023 20:37

perf: refactor interpreter internals (take 2)

9dae08e

perf: cast instruction functions to fn

7a9c5d6

The compiler generates much more favorable assembly if all the functions are casted to a `fn(_, _)` pointer before calling them. See <bluealloy#310 (comment)> for more information.

DaniPopes added 3 commits August 25, 2023 20:37

chore: remove prelude

6eaa0e7

perf: remove stack and memory bound checks on release

2d4818e

chore: re-add and deprecate Memory::get_slice

2510ae9

DaniPopes force-pushed the refactor-interpreter branch from 924e959 to 2510ae9 Compare August 25, 2023 18:37

DaniPopes commented Aug 25, 2023

View reviewed changes

DaniPopes marked this pull request as ready for review August 25, 2023 19:13

gakonst reviewed Aug 25, 2023

View reviewed changes

Merge branch 'main' into refactor-interpreter

7366887

rakita mentioned this pull request Aug 29, 2023

chore: remove all dyn for opcode that reduce dyn trait cost #660

Closed

thedevbirb mentioned this pull request Aug 31, 2023

Shared memory between calls #673

Merged

DaniPopes and others added 4 commits September 16, 2023 19:43

Merge branch 'main' into refactor-interpreter

24f9378

readd BLOBHASH

ff1c0c3

fix: TSTORE and TLOAD order

8f82eb8

some cleanup

eba8bfb

rakita reviewed Sep 19, 2023

View reviewed changes

rakita added 2 commits September 20, 2023 12:46

nits

0f2fdf9

Merge remote-tracking branch 'origin/main' into perf2

3ee1755

rakita approved these changes Sep 20, 2023

View reviewed changes

rakita merged commit d926728 into bluealloy:main Sep 20, 2023
6 of 8 checks passed

DaniPopes deleted the refactor-interpreter branch September 20, 2023 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: refactor interpreter internals (take 2) #582

perf: refactor interpreter internals (take 2) #582

DaniPopes commented Aug 3, 2023 •

edited

DaniPopes commented Aug 22, 2023 •

edited

DaniPopes left a comment

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023 •

edited

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

gakonst Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

DaniPopes Aug 25, 2023

gakonst Aug 25, 2023

DaniPopes Sep 16, 2023

gakonst left a comment •

edited

gakonst Aug 25, 2023

gakonst Aug 25, 2023

rakita Sep 19, 2023

DaniPopes Sep 19, 2023

rakita commented Sep 20, 2023

rakita left a comment

perf: refactor interpreter internals (take 2) #582

perf: refactor interpreter internals (take 2) #582

Conversation

DaniPopes commented Aug 3, 2023 • edited

Results

Cachegrind

DaniPopes commented Aug 22, 2023 • edited

DaniPopes 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

DaniPopes Aug 25, 2023 • edited

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

gakonst left a comment • edited

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

rakita commented Sep 20, 2023

rakita left a comment

Choose a reason for hiding this comment

DaniPopes commented Aug 3, 2023 •

edited

DaniPopes commented Aug 22, 2023 •

edited

DaniPopes Aug 25, 2023 •

edited

gakonst left a comment •

edited