Skip to content
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

Add Optional JIT compilation #91

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,6 @@ futures = "0.3"

# misc-testing
rstest = "0.18.2"

[patch.crates-io]
revmc = { path = "../revmc/crates/revmc" }
5 changes: 5 additions & 0 deletions crates/node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ alloy-primitives.workspace = true
serde_json.workspace = true
tracing.workspace = true
eyre.workspace = true
revmc = "0.1.0"


[lints]
workspace = true

[build-dependencies]
revmc-build = "0.1.0"
5 changes: 5 additions & 0 deletions crates/node/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#![allow(missing_docs)]

fn main() {
revmc_build::emit();
}
144 changes: 144 additions & 0 deletions crates/node/src/compiler.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
use alloy_primitives::{hex, Bytes, B256};
use core::panic;
use reth_primitives::Bytecode;
use reth_revm::{
handler::register::EvmHandler,
interpreter::{InterpreterAction, SharedMemory, EMPTY_SHARED_MEMORY},
Context as RevmContext, Database, Frame,
};
use revmc::{primitives::SpecId, EvmCompilerFn, EvmLlvmBackend, OptimizationLevel};
use std::{
collections::HashMap,
sync::{
mpsc::{Receiver, Sender},
Arc, Mutex,
},
thread,
};

#[derive(Debug)]
pub struct ExternalContext {
sender: Sender<(B256, Bytes)>,
// TODO: cache shouldn't be here (and should definitely not be wrapped in a mutex)
cache: Arc<Mutex<HashMap<B256, EvmCompilerFn>>>,
}

impl ExternalContext {
pub fn new(spec_id: SpecId) -> Self {
let cache = Arc::new(Mutex::new(HashMap::new()));
let (sender, receiver) = std::sync::mpsc::channel();

// TODO: graceful shutdown
thread::spawn({
let cache = cache.clone();

move || {
let context = Box::leak(Box::new(revmc::llvm::Context::create()));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I leak this, does that make sense? I guess I shouldn't need to since context will live for the whole while loop below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use with_llvm_context and nest the implementation inside of the closure

// TODO: fail properly here.
let backend =
EvmLlvmBackend::new(context, false, OptimizationLevel::Aggressive).unwrap();
let mut compiler = revmc::EvmCompiler::new(backend);

while let Ok((hash, code)) = receiver.recv() {
// Do we have to allocate here? Not sure there's a better option
let name = hex::encode(hash);
dbg!("compiled", &name);

let result =
unsafe { compiler.jit(&name, &code, spec_id) }.expect("catastrophe");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused as to the relationship between a compiler, a module, and a contract.
Should I be compiling all the code with the same compiler?


cache.lock().unwrap().insert(hash, result);

unsafe { compiler.clear().expect("could not clear") };
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this is wrong, as the functions that I'm storing in the hashmap are just linked back to this compiler. When I call clear and try to call the cached function, it's a use-after-free and I segfault.

I'm obviously thinking about using the compiler the wrong way, how should I be thinking about it?

I've read over your code here, but I believe that's for aot compilation instead of jit since you're writing everything out to files.

Should I just be creating a new compiler every time and storing the compiler as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just be creating a new compiler every time and storing the compiler as well?

Yes that's right

}
}
});

Self { sender, cache }
}

pub fn get_compiled_fn(&self, hash: B256, code: Bytes) -> Option<EvmCompilerFn> {
let Some(f) = self.cache.lock().unwrap().get(&hash).cloned() else {
self.sender.send((hash, code)).unwrap();
return None;
};

Some(f)
}
}

pub fn register_compiler_handler<DB>(handler: &mut EvmHandler<'_, ExternalContext, DB>)
where
DB: Database,
{
let f = handler.execution.execute_frame.clone();

handler.execution.execute_frame = Arc::new(move |frame, memory, table, context| {
let Some(action) = execute_frame(frame, memory, context) else {
dbg!("fallback");
return f(frame, memory, table, context);
};

Ok(action)
});
}

fn execute_frame<DB: Database>(
frame: &mut Frame,
memory: &mut SharedMemory,
context: &mut RevmContext<ExternalContext, DB>,
) -> Option<InterpreterAction> {
// let library = context.external.get_or_load_library(context.evm.spec_id())?;
let interpreter = frame.interpreter_mut();

let hash = match interpreter.contract.hash {
Some(hash) => hash,
None => unreachable_no_hash(),
};

// should be cheap enough to clone because it's backed by bytes::Bytes
let code = interpreter.contract.bytecode.bytes();

// TODO: put rules here for whether or not to compile the function
let f = context.external.get_compiled_fn(hash, code)?;

// let f = match library.get_function(hash) {
// Ok(Some(f)) => f,
// Ok(None) => return None,
// // Shouldn't happen.
// Err(err) => {
// unlikely_log_get_function_error(err, &hash);
// return None;
// }
// };

// interpreter.shared_memory =
// std::mem::replace(memory, reth_revm::interpreter::EMPTY_SHARED_MEMORY);
// let result = unsafe { f.call_with_interpreter(interpreter, context) };
// *memory = interpreter.take_memory();
// Some(result)

let result = unsafe { f.call_with_interpreter_and_memory(interpreter, memory, context) };

dbg!("executed", &hash);

Some(result)
}

#[cold]
#[inline(never)]
const fn unreachable_no_hash() -> ! {
panic!("unreachable: bytecode hash is not set in the interpreter")
}

#[cold]
#[inline(never)]
const fn unreachable_misconfigured() -> ! {
panic!("unreachable: AOT EVM is misconfigured")
}

#[cold]
#[inline(never)]
fn unlikely_log_get_function_error(err: impl std::error::Error, hash: &B256) {
tracing::error!(%err, %hash, "failed getting function from shared library");
}
15 changes: 13 additions & 2 deletions crates/node/src/evm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ use reth_revm::{
use revm_precompile::secp256r1;
use std::sync::Arc;

use crate::compiler::{self, register_compiler_handler};

/// Custom EVM configuration
#[derive(Debug, Clone)]
pub struct OdysseyEvmConfig {
Expand Down Expand Up @@ -221,12 +223,14 @@ impl ConfigureEvmEnv for OdysseyEvmConfig {
}

impl ConfigureEvm for OdysseyEvmConfig {
type DefaultExternalContext<'a> = ();
type DefaultExternalContext<'a> = compiler::ExternalContext;

fn evm<DB: Database>(&self, db: DB) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> {
EvmBuilder::default()
.with_db(db)
.optimism()
.reset_handler_with_external_context(self.default_external_context())
.append_handler_register(register_compiler_handler)
// add additional precompiles
.append_handler_register(Self::set_precompiles)
.build()
Expand All @@ -247,7 +251,14 @@ impl ConfigureEvm for OdysseyEvmConfig {
.build()
}

fn default_external_context<'a>(&self) -> Self::DefaultExternalContext<'a> {}
fn default_external_context<'a>(&self) -> Self::DefaultExternalContext<'a> {
let Self { chain_spec } = self;

// TODO: not sure if this is correct, I don't see a call-site for this method
let spec_id = revm_spec(chain_spec, &Head::default());

compiler::ExternalContext::new(spec_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec ID should also be dynamic and received from the channel

}
}

/// Determine the revm spec ID from the current block and reth chainspec.
Expand Down
1 change: 1 addition & 0 deletions crates/node/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@
#![warn(unused_crate_dependencies)]

pub mod chainspec;
pub mod compiler;
pub mod evm;
pub mod node;