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.

Sign up for GitHub

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

debug execution: implement actor redirects in engine #605

Merged
merged 2 commits into from
Jun 21, 2022
Merged

debug execution: implement actor redirects in engine #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c5515c
implement actor redirects in engine
vyzo Jun 2, 2022
45a9e9e
fix clippy
vyzo Jun 10, 2022
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
25 changes: 25 additions & 0 deletions fvm/src/machine/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@ pub struct MultiEngine(Arc<Mutex<HashMap<EngineConfig, Engine>>>);
pub struct EngineConfig {
pub max_wasm_stack: u32,
pub wasm_prices: &'static WasmGasPrices,
pub actor_redirect: Option<Vec<(Cid, Cid)>>,
}

impl From<&NetworkConfig> for EngineConfig {
fn from(nc: &NetworkConfig) -> Self {
EngineConfig {
max_wasm_stack: nc.max_wasm_stack,
wasm_prices: &nc.price_list.wasm_rules,
actor_redirect: nc.actor_redirect.clone(),
}
}
}
Expand Down Expand Up @@ -158,6 +160,8 @@ struct EngineInner {
module_cache: Mutex<HashMap<Cid, Module>>,
instance_cache: Mutex<anymap::Map<dyn anymap::any::Any + Send>>,
config: EngineConfig,

actor_redirect: Option<HashMap<Cid, Cid>>,
}

impl Deref for Engine {
Expand Down Expand Up @@ -185,13 +189,19 @@ impl Engine {
let dummy_memory = Memory::new(&mut dummy_store, MemoryType::new(0, Some(0)))
.expect("failed to create dummy memory");

let actor_redirect = ec
.actor_redirect
.as_ref()
.map(|v| v.iter().cloned().collect());
Comment on lines +194 to +195
Copy link
Member

Choose a reason for hiding this comment

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

If we're not overriding anything, we might as well just have an empty hashmap.

Suggested change
.as_ref()
.map(|v| v.iter().cloned().collect());
.as_ref()
.unwrap_or_default().iter().cloned().collect();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, do you want to drop the option from the data structure?
I thought it would be faster to None check instead of a get in an empty map, but if there is no difference we might 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.

An option prevents us from hashing on access when there are no redirects set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so lets keep it and decline steb's suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

An option prevents us from hashing on access when there are no redirects set.

Rust is smarter than that https://github.com/rust-lang/hashbrown/blob/f3a242fb48e73df704d27c9722e30ebda5dd8e4f/src/map.rs#L1099-L1104


Ok(Engine(Arc::new(EngineInner {
engine,
dummy_memory,
dummy_gas_global: dummy_gg,
module_cache: Default::default(),
instance_cache: Mutex::new(anymap::Map::new()),
config: ec,
actor_redirect,
})))
}
}
Expand All @@ -211,6 +221,7 @@ impl Engine {
{
let mut cache = self.0.module_cache.lock().expect("module_cache poisoned");
for cid in cids {
let cid = self.with_redirect(cid);
if cache.contains_key(cid) {
continue;
}
Expand All @@ -226,8 +237,19 @@ impl Engine {
Ok(())
}

fn with_redirect<'a>(&'a self, k: &'a Cid) -> &'a Cid {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this explicit lifetime?

Copy link
Contributor Author

@vyzo vyzo Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yes, i refuses to compile without it.

match &self.0.actor_redirect {
Some(tab) => match tab.get(k) {
Some(cid) => cid,
None => k,
},
None => k,
}
}

/// Load some wasm code into the engine.
pub fn load_bytecode(&self, k: &Cid, wasm: &[u8]) -> anyhow::Result<Module> {
let k = self.with_redirect(k);
let mut cache = self.0.module_cache.lock().expect("module_cache poisoned");
let module = match cache.get(k) {
Some(module) => module.clone(),
Expand Down Expand Up @@ -287,6 +309,7 @@ impl Engine {
///
/// See [`wasmtime::Module::deserialize`] for safety information.
pub unsafe fn load_compiled(&self, k: &Cid, compiled: &[u8]) -> anyhow::Result<Module> {
let k = self.with_redirect(k);
let mut cache = self.0.module_cache.lock().expect("module_cache poisoned");
let module = match cache.get(k) {
Some(module) => module.clone(),
Expand All @@ -301,6 +324,7 @@ impl Engine {

/// Lookup a loaded wasmtime module.
pub fn get_module(&self, k: &Cid) -> Option<Module> {
let k = self.with_redirect(k);
self.0
.module_cache
.lock()
Expand All @@ -316,6 +340,7 @@ impl Engine {
store: &mut wasmtime::Store<InvocationData<K>>,
k: &Cid,
) -> anyhow::Result<Option<wasmtime::Instance>> {
let k = self.with_redirect(k);
let mut instance_cache = self.0.instance_cache.lock().expect("cache poisoned");

let cache = match instance_cache.entry() {
Expand Down
10 changes: 10 additions & 0 deletions fvm/src/machine/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ pub struct NetworkConfig {
///
/// DEFAULT: The price-list for the current network version.
pub price_list: &'static PriceList,

/// Actor redirects for debug execution
pub actor_redirect: Option<Vec<(Cid, Cid)>>,
Copy link
Member

Choose a reason for hiding this comment

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

No reason to make this an option either. An empty vector is free.

}

impl NetworkConfig {
Expand All @@ -128,6 +131,7 @@ impl NetworkConfig {
actor_debugging: false,
builtin_actors_override: None,
price_list: price_list_by_network_version(network_version),
actor_redirect: None,
}
}

Expand All @@ -145,6 +149,12 @@ impl NetworkConfig {
self
}

/// Set actor redirects for debug execution
pub fn redirect_actors(&mut self, actor_redirect: Vec<(Cid, Cid)>) -> &mut Self {
self.actor_redirect = Some(actor_redirect);
self
}

/// Create a [`MachineContext`] for a given `epoch` with the specified `initial_state`.
pub fn for_epoch(&self, epoch: ChainEpoch, initial_state: Cid) -> MachineContext {
MachineContext {
Expand Down