Feature/engine no std #595

iamyulong · 2022-11-18T17:44:09Z

More and more non-std incompatible changes have been introduced into the code base.

To prevent further diverge, this PR restores no_std build and adds it to CI.

dhedey

I'm not sure if this is ready for review yet, but it's not a draft PR, so as I noticed it, I've taken a look.

I approve of the want to restore a no-std build for the engine, but I worry about a few of the details here. Happy to chat if you'd like :)

dhedey · 2022-11-20T15:13:38Z

radix-engine/src/wasm/wasm_instrumenter.rs

-            .build();
+    pub fn new(_options: InstrumenterOptions) -> Self {
+        // TODO: limit size
+        let cache = RefCell::new(HashMap::new());


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?

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?

Mutex is not part of the no_std

Let me try conditinal compiling first.

dhedey · 2022-11-20T15:15:33Z

radix-engine/src/wasm/wasm_instrumenter.rs


 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>>>>,


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?

Mutex is not part of the no_std.

dhedey · 2022-11-20T15:27:45Z

radix-engine/src/engine/interpreters/scrypto_interpreter.rs

@@ -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!


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? 👍

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.

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?

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

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.

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.

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.

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). :)

iamyulong · 2022-11-21T12:19:35Z

It's now ready for another review. @dhedey

dhedey

Nice, thanks! 👍

Happy for you to merge as-is or to extract out a separate Cache type.

EDIT: Looks like you still need to fix some failing tests: https://github.com/radixdlt/radixdlt-scrypto/actions/runs/3515933512/jobs/5891823809

dhedey · 2022-11-21T22:13:03Z

radix-engine/Cargo.toml

 indexmap = { version = "1.8.1" }
-moka = { version = "0.9.4", features = ["sync"], default-features = false }
+lru = { version = "0.8.1", default-features = false }


Should we put lru behind optional too, and only enable it in the alloc case?

Honestly, we don't gain much from this (tiny compile time of a small crate) by coupling lru and alloc. A slightly better approach is to make it a feature if we really care. But, will punt on this for now. :)

dhedey · 2022-11-21T22:14:35Z

radix-engine/src/engine/interpreters/scrypto_interpreter.rs

@@ -116,6 +116,8 @@ mod tests {
        // RFC 2056
        fn assert_all() {
            assert_send::<ScryptoInterpreter<DefaultWasmEngine>>();
+            // TODO: make sure engine is indeed multi-thread safe!


When you say the engine, do you mean the DefaultWasmEngine? or the RadixEngine itself?

See my previous comment. Do you have specific worries or things you want to check? Else I think this code comment should probably be removed.

iamyulong added 3 commits November 18, 2022 17:41

Restore radix engine no_std

0b834f6

Add no_std engine test

f875ce7

Format code

1dad679

dhedey requested changes Nov 20, 2022

View reviewed changes

dhedey reviewed Nov 20, 2022

View reviewed changes

iamyulong added 3 commits November 21, 2022 10:51

Merge remote-tracking branch 'origin/develop' into feature/engine-no-std

9041616

Fix wasmer build

40a2f3f

Add LruCache

16dce1f

iamyulong force-pushed the feature/engine-no-std branch from 84ef28b to 16dce1f Compare November 21, 2022 11:23

Conditionally enable moka

35043fe

iamyulong force-pushed the feature/engine-no-std branch 2 times, most recently from d7b6cdc to 92966d0 Compare November 21, 2022 12:00

Re-enable assert_sync test for no_std, keeping todo

1a7a7f9

iamyulong force-pushed the feature/engine-no-std branch from 92966d0 to 1a7a7f9 Compare November 21, 2022 12:10

Merge remote-tracking branch 'origin/develop' into feature/engine-no-std

b59c4f5

dhedey approved these changes Nov 21, 2022

View reviewed changes

iamyulong added 2 commits November 22, 2022 09:21

Merge remote-tracking branch 'origin/develop' into feature/engine-no-std

b0b721f

Fix post-merge

1423e95

iamyulong merged commit 97a3a61 into develop Nov 22, 2022

talekhinezh deleted the feature/engine-no-std branch February 21, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/engine no std #595

Feature/engine no std #595

iamyulong commented Nov 18, 2022 •

edited

Loading

dhedey left a comment

dhedey Nov 20, 2022 •

edited

Loading

dhedey Nov 20, 2022

iamyulong Nov 21, 2022 •

edited

Loading

iamyulong Nov 21, 2022 •

edited

Loading

dhedey Nov 20, 2022

iamyulong Nov 21, 2022

dhedey Nov 20, 2022

iamyulong Nov 21, 2022

dhedey Nov 21, 2022 •

edited

Loading

iamyulong Nov 22, 2022

dhedey Nov 23, 2022

dhedey Nov 23, 2022 •

edited

Loading

talekhinezh Nov 24, 2022

dhedey Nov 24, 2022

iamyulong commented Nov 21, 2022

dhedey left a comment •

edited

Loading

dhedey Nov 21, 2022

iamyulong Nov 22, 2022

dhedey Nov 21, 2022

dhedey Nov 21, 2022 •

edited

Loading

Feature/engine no std #595

Feature/engine no std #595

Conversation

iamyulong commented Nov 18, 2022 • edited Loading

dhedey left a comment

Choose a reason for hiding this comment

dhedey Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamyulong Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

iamyulong Nov 21, 2022 • edited Loading

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

dhedey Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamyulong commented Nov 21, 2022

dhedey left a comment • edited Loading

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

dhedey Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

iamyulong commented Nov 18, 2022 •

edited

Loading

dhedey Nov 20, 2022 •

edited

Loading

iamyulong Nov 21, 2022 •

edited

Loading

iamyulong Nov 21, 2022 •

edited

Loading

dhedey Nov 21, 2022 •

edited

Loading

dhedey Nov 23, 2022 •

edited

Loading

dhedey left a comment •

edited

Loading

dhedey Nov 21, 2022 •

edited

Loading