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

Feature/engine no std #595

Merged
merged 11 commits into from
Nov 22, 2022
Merged

Feature/engine no std #595

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0b834f6
Restore radix engine no_std
iamyulong Nov 18, 2022
f875ce7
Add no_std engine test
iamyulong Nov 18, 2022
1dad679
Format code
iamyulong Nov 18, 2022
9041616
Merge remote-tracking branch 'origin/develop' into feature/engine-no-std
iamyulong Nov 21, 2022
40a2f3f
Fix wasmer build
iamyulong Nov 21, 2022
16dce1f
Add LruCache
iamyulong Nov 21, 2022
35043fe
Conditionally enable moka
iamyulong Nov 21, 2022
1a7a7f9
Re-enable assert_sync test for no_std, keeping todo
iamyulong Nov 21, 2022
b59c4f5
Merge remote-tracking branch 'origin/develop' into feature/engine-no-std
iamyulong Nov 21, 2022
b0b721f
Merge remote-tracking branch 'origin/develop' into feature/engine-no-std
iamyulong Nov 22, 2022
1423e95
Fix post-merge
iamyulong Nov 22, 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
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ jobs:
- name: Run tests
run: cargo test
working-directory: radix-engine
- name: Run tests (no_std)
run: cargo test --no-default-features --features alloc
working-directory: radix-engine
radix-engine-wasmer:
name: Run Radix Engine tests with Wasmer
runs-on: ${{ matrix.os }}
Expand Down
4 changes: 0 additions & 4 deletions radix-engine-derive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ proc-macro2 = { version = "1.0.38" }
syn = { version = "1.0.93", features = ["full", "extra-traits"] }
quote = { version = "1.0.18" }
uuid = { version = "1.0.0", features = ["v4"] }
serde = { version = "1.0.137", default-features = false }
serde_json = { version = "1.0.81", default-features = false }
sbor = { path = "../sbor", default-features = false }
scrypto-abi = { path = "../scrypto-abi", default-features = false }

Expand All @@ -29,12 +27,10 @@ scrypto-abi = { path = "../scrypto-abi", default-features = false }
#
default = ["std"]
std = [
"serde/std", "serde_json/std",
"sbor/std", "sbor/serde",
"scrypto-abi/std", "scrypto-abi/serde"
]
alloc = [
"serde/alloc", "serde_json/alloc",
"sbor/alloc", "sbor/serde",
"scrypto-abi/alloc", "scrypto-abi/serde"
]
Expand Down
10 changes: 9 additions & 1 deletion radix-engine-stores/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,18 @@ version = "0.7.0"
edition = "2021"

[dependencies]
sbor = { path = "../sbor", default-features = false }
radix-engine = { path = "../radix-engine" }
radix-engine-interface = { path = "../radix-engine-interface", default-features = false }
sbor = { path = "../sbor" }
rocksdb = { version = "0.19.0", optional = true }

[features]
default = ["std"]
std = [
"sbor/std", "radix-engine-interface/std",
]
alloc = [
"sbor/alloc","radix-engine-interface/alloc",
]

rocksdb = ["dep:rocksdb"]
7 changes: 3 additions & 4 deletions radix-engine/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ colored = { version = "2.0.0", default-features = false }
hex = { version = "0.4.3", default-features = false }
bitflags = "1.3"
indexmap = { version = "1.8.1" }
moka = { version = "0.9.4", features = ["sync"], default-features = false }

# WASM de-/serialization
parity-wasm = { version = "0.42.2" }
Expand All @@ -35,7 +34,7 @@ wasmer-compiler-singlepass = { version = "2.2.1", optional = true }
[dev-dependencies]
wabt = { version = "0.10.0" }
criterion = { version = "0.3", features = ["html_reports"] }
scrypto-unit = { path = "../scrypto-unit" }
scrypto-unit = { path = "../scrypto-unit", default-features = false }
rand = { version = "0.8.5" }
rand_chacha = { version = "0.3.1" }
rayon = "1.5.3"
Expand All @@ -55,8 +54,8 @@ harness = false
[features]
# You should enable either `std` or `alloc`
default = ["std"]
std = ["sbor/std", "scrypto/std", "wasmi/std", "transaction/std"]
alloc = ["sbor/alloc", "scrypto/alloc", "transaction/alloc"]
std = ["sbor/std", "scrypto/std", "scrypto-unit/std", "wasmi/std", "transaction/std", "radix-engine-interface/std", "utils/std"]
alloc = ["sbor/alloc", "scrypto/alloc", "scrypto-unit/alloc", "transaction/alloc", "radix-engine-interface/alloc", "utils/alloc"]

# Use `wasmer` as WASM engine, otherwise `wasmi`
wasmer = ["dep:wasmer", "dep:wasmer-compiler-singlepass"]
3 changes: 1 addition & 2 deletions radix-engine/benches/wasm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use std::sync::Arc;

use criterion::{criterion_group, criterion_main, Criterion};
use radix_engine::model::extract_abi;
use radix_engine::wasm::DefaultWasmEngine;
use radix_engine::wasm::InstrumentedCode;
use radix_engine::wasm::WasmEngine;
use radix_engine::wasm::WasmValidator;
use radix_engine_interface::crypto::hash;
use sbor::rust::sync::Arc;

fn bench_wasm_validation(c: &mut Criterion) {
let code = include_bytes!("../../assets/account.wasm");
Expand Down
2 changes: 2 additions & 0 deletions radix-engine/src/engine/interpreters/scrypto_interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ use radix_engine_interface::api::types::{
};
use radix_engine_interface::crypto::Hash;
use radix_engine_interface::data::IndexedScryptoValue;
use sbor::rust::string::String;
use sbor::rust::vec::Vec;

impl<'g, 's, W, R, N, T> SysNativeInvokable<N, RuntimeError> for Kernel<'g, 's, W, R>
where
Expand Down
3 changes: 2 additions & 1 deletion radix-engine/src/engine/interpreters/scrypto_interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ mod tests {
// RFC 2056
fn assert_all() {
assert_send::<ScryptoInterpreter<DefaultWasmEngine>>();
assert_sync::<ScryptoInterpreter<DefaultWasmEngine>>();
// TODO: make sure engine is multi-thread safe!
Copy link
Contributor

Choose a reason for hiding this comment

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

We create a new Radix engine for each transaction and single thread it - it's the ScryptoInterpreter caches we're sharing.

#FearlessConcurrency should mean this is fine? 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

As the runtime shares pointer to the cache, they may run into illegal state due to memory volatility. Like I said, we need dedicated story to make sure multi-thread work rather pretend to.

Copy link
Contributor

@dhedey dhedey Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I mean I have already verified this - in the very least:

  • The moka cache claims to be thread safe
  • The multiple copies of the engine share the WasmModule which is just (a reference to) data, and from which they instantiate separate / independent WasmInstances

Separately, the only other interaction between engine threads is in the database layer - where RocksDB is threadsafe. (and we ensure there are no commits during execution of other transactions)

Is there anything I've missed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly the wasmer implementation which stores a pointer which could be volatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that's the WasmerInstance which contains the pointer, not the WasmerModule. The WasmerInstance is not shared - it's created for each execution from the (shared) WasmerModule template.

Copy link
Contributor

@dhedey dhedey Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed - Wasmer is written in Rust, and they have good understanding of Send/Sync (It appears even Instance implements send!), so we can trust that if Module is Sync then it is Sync. Indeed - I believe it's essentially a data object.

Copy link
Member

Choose a reason for hiding this comment

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

You don't know what Rust is doing with the data under that pointer though since Rust doesn't know that it's a mutable object potentially being written to across multiple threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you're referring to the pointer that we create ourselves. This pointer isn't in the object which is being shared. This pointer is in the WasmerInstance (not shared) not the WasmerModule (shared).

This pointer in WasmerInstance has no affect on whether WasmerModule is Sync.

On a related note, I'm quite surprised that we use runtime_ptr: Arc<Mutex<usize>>, and use usize, not a raw pointer type here, but anyway... I agree WasmerInstance possibly has an unsound impl of Sync (without looking into it further)... But feeling slightly like a broken record, we're not sharing WasmerInstance. And I've double-checked - the WasmerInstance pointer does not point into WasmerModule (which would be unsound for sure if it did). :)

// assert_sync::<ScryptoInterpreter<DefaultWasmEngine>>();
}
};
}
1 change: 1 addition & 0 deletions radix-engine/src/engine/interpreters/wasm_runtime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use radix_engine_interface::api::wasm_input::{
ResourceManagerMethodInvocation, VaultMethodInvocation, WorktopMethodInvocation,
};
use radix_engine_interface::data::{IndexedScryptoValue, ScryptoCustomTypeId};
use sbor::rust::vec::Vec;

/// A glue between system api (call frame and track abstraction) and WASM.
///
Expand Down
4 changes: 2 additions & 2 deletions radix-engine/src/engine/kernel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ use radix_engine_interface::api::types::{
use radix_engine_interface::crypto::Hash;
use radix_engine_interface::data::*;

use sbor::rust::fmt::Debug;
use sbor::rust::mem;
use scrypto::access_rule_node;
use scrypto::rule;
use std::fmt::Debug;
use std::mem;
use transaction::errors::IdAllocationError;
use transaction::model::AuthZoneParams;
use transaction::validation::*;
Expand Down
2 changes: 1 addition & 1 deletion radix-engine/src/engine/modules/execution_trace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use radix_engine_interface::api::types::{
use radix_engine_interface::data::IndexedScryptoValue;
use radix_engine_interface::math::Decimal;
use radix_engine_interface::model::*;
use std::fmt::Debug;
use sbor::rust::fmt::Debug;

#[derive(Debug, Clone, PartialEq, Eq)]
#[scrypto(TypeId, Encode, Decode)]
Expand Down
4 changes: 2 additions & 2 deletions radix-engine/src/model/kv_store/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use sbor::rust::collections::HashMap;

use crate::model::KeyValueStoreEntrySubstate;
use sbor::rust::collections::HashMap;
use sbor::rust::vec::Vec;

#[derive(Debug)]
pub struct KeyValueStore {
Expand Down
1 change: 1 addition & 0 deletions radix-engine/src/model/method_authorization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use radix_engine_interface::math::Decimal;
use radix_engine_interface::model::*;
use sbor::rust::vec::Vec;
use sbor::*;
use scrypto::scrypto;

Expand Down
2 changes: 1 addition & 1 deletion radix-engine/src/model/resources/substates/vault.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::model::{
ResourceOperationError, VaultError,
};
use crate::types::*;
use std::ops::Deref;
use sbor::rust::ops::Deref;

#[derive(Debug, Clone, PartialEq, Eq)]
#[scrypto(TypeId, Encode, Decode)]
Expand Down
34 changes: 18 additions & 16 deletions radix-engine/src/transaction/preview_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,24 @@ pub fn execute_preview<S: ReadableSubstateStore, W: WasmEngine, IHM: IntentHashM

let validator = NotarizedTransactionValidator::new(validation_config);

let executable = validator
.validate_preview_intent(&preview_intent, intent_hash_manager)
.map_err(PreviewError::TransactionValidationError)?;

let mut fee_reserve = SystemLoanFeeReserve::default();
if preview_intent.flags.unlimited_loan {
fee_reserve.credit(PREVIEW_CREDIT);
}

let receipt = execute_transaction_with_fee_reserve(
substate_store,
scrypto_interpreter,
SystemLoanFeeReserve::default(),
&ExecutionConfig::default(),
&executable,
);
let receipt = {
let executable = validator
.validate_preview_intent(&preview_intent, intent_hash_manager)
.map_err(PreviewError::TransactionValidationError)?;

let mut fee_reserve = SystemLoanFeeReserve::default();
if preview_intent.flags.unlimited_loan {
fee_reserve.credit(PREVIEW_CREDIT);
}

execute_transaction_with_fee_reserve(
substate_store,
scrypto_interpreter,
SystemLoanFeeReserve::default(),
&ExecutionConfig::default(),
&executable,
)
};

Ok(PreviewResult {
intent: preview_intent,
Expand Down
2 changes: 0 additions & 2 deletions radix-engine/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ pub use sbor::{Decode, DecodeError, Encode, SborPath, SborPathBuf, SborTypeId, S

pub use scrypto::abi::{BlueprintAbi, Fields, Fn, Type, Variant};

use std::fmt::Debug;

// methods and macros
use crate::engine::Invocation;

Expand Down
3 changes: 1 addition & 2 deletions radix-engine/src/wasm/cost_rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ impl Rules for InstructionCostRules {
mod tests {
use super::*;
use crate::wasm::WasmModule;
use wabt::{wasm2wat, wat2wasm};
use wabt::wat2wasm;

#[test]
fn test_cost_rules() {
Expand Down Expand Up @@ -271,7 +271,6 @@ mod tests {
.to_bytes()
.unwrap()
.0;
println!("{}", wasm2wat(&transformed).unwrap());

// Costs:
// 12 = 10 (local.get) + 1 (i32.const) + 1 (i32.mul)
Expand Down
7 changes: 3 additions & 4 deletions radix-engine/src/wasm/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use super::InstrumentedCode;
use crate::model::InvokeError;
use crate::wasm::errors::*;
use radix_engine_interface::data::IndexedScryptoValue;
use sbor::rust::boxed::Box;

use crate::wasm::errors::*;

use super::InstrumentedCode;
use sbor::rust::vec::Vec;

/// Represents the runtime that can be invoked by Scrypto modules.
pub trait WasmRuntime {
Expand Down
38 changes: 18 additions & 20 deletions radix-engine/src/wasm/wasm_instrumenter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
use moka::sync::Cache;
use radix_engine_interface::crypto::hash;
use std::sync::Arc;
use sbor::rust::cell::RefCell;
use sbor::rust::collections::HashMap;
use sbor::rust::sync::Arc;

use crate::types::*;
use crate::wasm::{WasmMeteringConfig, WasmModule};

pub struct WasmInstrumenter {
cache: Cache<(Hash, Hash), Arc<Vec<u8>>>,
cache: RefCell<HashMap<(Hash, Hash), Arc<Vec<u8>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will panic if multiple threads try to lock it at the same time - as can happen in the Node when we are accepting multiple mempool submissions at once.

Can we wrap it in a Mutex instead of a RefCell?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mutex is not part of the no_std.

}

#[derive(Debug, Clone)]
pub struct InstrumenterOptions {
#[allow(dead_code)]
max_cache_size_bytes: u64,
}

Expand All @@ -28,17 +30,9 @@ pub struct InstrumentedCode {
}

impl WasmInstrumenter {
pub fn new(options: InstrumenterOptions) -> Self {
let cache = Cache::builder()
.weigher(|_key: &(Hash, Hash), value: &Arc<Vec<u8>>| -> u32 {
value
.len()
.checked_add(Hash::LENGTH * 2)
.and_then(|total| total.try_into().ok())
.unwrap_or(u32::MAX)
})
.max_capacity(options.max_cache_size_bytes)
.build();
pub fn new(_options: InstrumenterOptions) -> Self {
// TODO: limit size
let cache = RefCell::new(HashMap::new());
Copy link
Contributor

@dhedey dhedey Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This feels like a definite reversion of behaviour. We want a safe and performant cache implementation here, for use in the node.

Can we instead just fall back to this basic version in the no-std case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Better yet, what about creating our own ThreadsafeCache type, and swap out the implementation for either wrapping Moka or a Mutex<HashMap<K, Arc>> depending on if we use alloc or not?

Copy link
Member Author

@iamyulong iamyulong Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Mutex is not part of the no_std

Copy link
Member Author

@iamyulong iamyulong Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let me try conditinal compiling first.


Self { cache }
}
Expand All @@ -51,16 +45,20 @@ impl WasmInstrumenter {
let code_hash = hash(code);
let cache_key = (code_hash, *wasm_metering_config.identifier());

if let Some(cached) = self.cache.get(&cache_key) {
return InstrumentedCode {
code: cached.clone(),
code_hash,
};
{
if let Some(cached) = self.cache.borrow().get(&cache_key) {
return InstrumentedCode {
code: cached.clone(),
code_hash,
};
}
}

let instrumented_ref = Arc::new(self.instrument_no_cache(code, wasm_metering_config));

self.cache.insert(cache_key, instrumented_ref.clone());
self.cache
.borrow_mut()
.insert(cache_key, instrumented_ref.clone());

InstrumentedCode {
code: instrumented_ref,
Expand Down
3 changes: 1 addition & 2 deletions radix-engine/src/wasm/wasmer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use std::sync::{Arc, Mutex};

use crate::model::InvokeError;
use moka::sync::Cache;
use radix_engine_interface::data::IndexedScryptoValue;
use sbor::rust::sync::{Arc, Mutex};
use wasmer::{
imports, Function, HostEnvInitError, Instance, LazyInit, Module, RuntimeError, Store,
Universal, Val, WasmerEnv,
Expand Down
Loading