refactor: reinstate module cache #106

jost-s · 2024-01-05T18:03:14Z

Instance cache in host crate has been removed in favor of a deserialized module cache DeserializedModuleCache. An abstraction for caching (serialized & deserialized modules) called ModuleCache was added.
All logic related to modules and wasmer caching from holochain has been moved to the host crate. Consequently functions for wasmer development under iOS need to be imported from here.

jost-s · 2024-01-10T21:29:13Z

crates/host/src/module.rs

+#[cfg(not(test))]
+/// one hundred giga ops
+pub const WASM_METERING_LIMIT: u64 = 100_000_000_000;
+
+#[cfg(test)]
+/// ten mega ops.
+/// We don't want tests to run forever, and it can take several minutes for 100 giga ops to run.
+pub const WASM_METERING_LIMIT: u64 = 10_000_000;
+
+/// Generate an engine with a wasm compiler
+/// and Metering (use limits) in place.
+pub fn make_compiler_engine() -> Engine {
+    let cost_function = |_operator: &wasmparser::Operator| -> u64 { 1 };
+    // @todo 100 giga-ops is totally arbitrary cutoff so we probably
+    // want to make the limit configurable somehow.
+    let metering = Arc::new(Metering::new(WASM_METERING_LIMIT, cost_function));
+    // the only place where the wasm compiler engine is set
+    let mut compiler = Cranelift::default();
+    compiler.canonicalize_nans(true).push_middleware(metering);
+    Engine::from(compiler)
+}
+
+/// Generate a runtime `Engine` without compiler suitable for iOS.
+/// Useful for re-building an iOS Module from a preserialized WASM Module.
+pub fn make_ios_runtime_engine() -> Engine {
+    Engine::headless()
+}
+
+/// Take WASM binary and prepare a wasmer Module suitable for iOS
+pub fn build_ios_module(wasm: &[u8]) -> Result<Module, CompileError> {
+    info!(
+        "Found wasm and was instructed to serialize it for ios in wasmer format, doing so now..."
+    );
+    let compiler_engine = make_compiler_engine();
+    let store = Store::new(compiler_engine);
+    Module::from_binary(&store, wasm)
+}
+
+/// Deserialize a previously compiled module for iOS from a file.
+pub fn get_ios_module_from_file(path: &PathBuf) -> Result<Module, DeserializeError> {
+    let engine = make_ios_runtime_engine();
+    unsafe { Module::deserialize_from_file(&engine, path) }
+}
+
+/// Configuration of a Target for wasmer for iOS
+pub fn wasmer_ios_target() -> Target {
+    // use what I see in
+    // platform ios headless example
+    // https://github.com/wasmerio/wasmer/blob/447c2e3a152438db67be9ef649327fabcad6f5b8/examples/platform_ios_headless.rs#L38-L53
+    let triple = Triple::from_str("aarch64-apple-ios").unwrap();
+    let cpu_feature = CpuFeature::set();
+    Target::new(triple, cpu_feature)
+}
+


All these items come from holochain_types::wasmer_types

jost-s · 2024-01-10T21:29:44Z

crates/host/src/module.rs

@@ -128,7 +192,12 @@ pub struct SerializedModuleCache {
    pub plru: MicroCache,
    pub key_map: PlruKeyMap,
    pub cache: BTreeMap<CacheKey, Arc<SerializedModule>>,
-    pub cranelift: fn() -> Cranelift,
+    // a function to create a new compiler engine for every module
+    pub make_compiler_engine: fn() -> Engine,


To keep this independent of the compiler, I changed it return an engine

jost-s · 2024-01-10T21:30:22Z

crates/host/src/module.rs

-    pub fn default_with_cranelift(cranelift: fn() -> Cranelift) -> Self {
+    /// Build a default `SerializedModuleCache` with a fn to create an `Engine`
+    /// that will be used to compile modules from wasms as needed.
+    pub fn default_with_engine(


arguably this can be renamed to new_with_engine or just new

Sounds reasonable to me, is this a breaking change anyway? If so I'd go for the rename. Otherwise maybe leave a comment behind to do it later?

jost-s · 2024-01-10T21:32:12Z

test/benches/bench.rs

+    // wasm_module,
+    // wasm_instance,


currently the bench fails because such numerous deserialization of modules causes memory leaks because of an upstream issue where the memory for deserialization is kept as long as the engine lives wasmerio/wasmer#4377 (comment)

This shouldn't affect Holochain in practice because we're only deserializing every module once.

Can a note be left in the code here so we don't have to trace back to this PR to figure out why these are commented out later?

ThetaSinner

Looks good to me, have left a few comments but nothing that really concerns me so I'll approve and leave them with you :)

ThetaSinner · 2024-01-10T23:06:54Z

Cargo.toml

@@ -1,3 +1,5 @@
 [workspace]
 members = ["crates/host", "crates/common"]
 exclude = ["test"]
+
+resolver = "2"


Thanks! Makes debugging feature problems so much easier

ThetaSinner · 2024-01-10T23:08:38Z

crates/host/src/module.rs

+
+/// Generate a runtime `Engine` without compiler suitable for iOS.
+/// Useful for re-building an iOS Module from a preserialized WASM Module.
+pub fn make_ios_runtime_engine() -> Engine {


Should these be behind a platform flag so they aren't available everywhere? Or are they valid anywhere but just recommended for iOS?

Actually I think even in the second case, if they have ios in the name I'd be in favour of them being behind a platform condition

They haven't been behind a feature flag before inside holochain, I just ported these over.

Understood, I still think they probably ought to be but as this was just moved code it can wait for a less busy time :)

Another limiting factor is that where they're used in holochain is in an if..else assignment, so I think we can't hide a part of it behind a feature. https://github.com/holochain/holochain/pull/3152/files#diff-14f820cc8b4334c432a642d67c2e4de0fb36249b4dd73b8bf6124482abc45427R350-R356

ThetaSinner · 2024-01-10T23:09:39Z

crates/host/src/module.rs

-    pub fn default_with_cranelift(cranelift: fn() -> Cranelift) -> Self {
+    /// Build a default `SerializedModuleCache` with a fn to create an `Engine`
+    /// that will be used to compile modules from wasms as needed.
+    pub fn default_with_engine(


Sounds reasonable to me, is this a breaking change anyway? If so I'd go for the rename. Otherwise maybe leave a comment behind to do it later?

ThetaSinner · 2024-01-10T23:10:49Z

crates/host/src/module.rs

+                (deserialized_module, serialized_module)
+            }
+            // no serialized module found on the file system, so serialize the
+            // wasm binary and write it to the file system
            _fs_miss => {


Is a file system miss the only option here? What about other errors in a Some(Err(_))?

I know this is existing code, just checking while we're here and looking at it

In case of a Some(Err) the err will be swallowed, something to improve I agree

ThetaSinner · 2024-01-10T23:12:51Z

crates/host/src/module.rs

+}
+
+#[test]
+fn cache_test() {


Should these tests be in a module that is only available in test config?

Also, YAY tests! :)

I wonder too

Ahh okay, again this is an existing pattern for this create. Understood, something for another time!

@ThetaSinner @jost-s uh no? there are tests in the test folder, why is this here? 😅

@thedavidmeister To have access to the member fields. If I put the test in the tests folder, I need to make the member fields public.

@jost-s ah, so then can we put it behind a cfg?

@thedavidmeister done

ThetaSinner · 2024-01-10T23:13:49Z

test/benches/bench.rs

+    // wasm_module,
+    // wasm_instance,


Can a note be left in the code here so we don't have to trace back to this PR to figure out why these are commented out later?

thedavidmeister

only thing is i'm not sure why there are tests in the main crates rather than test crates

thedavidmeister · 2024-01-16T16:15:33Z

crates/host/src/module.rs

+}
+
+#[test]
+fn cache_test() {


@ThetaSinner @jost-s uh no? there are tests in the test folder, why is this here? 😅

jost-s added 15 commits December 18, 2023 20:10

chore: set resolver to v2

4ca1d96

refactor: reinstate module cache

74e689b

test: fix module caching

f64a928

chore: update host package version to 0.0.91

55f7b99

test(host): add test for cache

e6679d1

refactor: move all wasmer-types related items to holochain-wasmer

5eb7ce4

refactor(host): change identifier names according to refactors

7f4ce38

docs: update changelog

d663a56

test: fix module creation

29409f7

test: comment out bench wasm_instance due to upstream memory leak

febc87c

test: comment out bench wasm_module due to upstream memory leak

7c39692

refactor: move ios precompiled module getter to holochain-wasmer

3aa836d

refactor: rename precompiled module fn

6351ca2

build: switch to llvm compiler

1624030

revert switch to llvm

033be03

jost-s commented Jan 10, 2024

View reviewed changes

jost-s marked this pull request as ready for review January 10, 2024 21:35

jost-s requested review from thedavidmeister, ThetaSinner and maackle January 10, 2024 21:35

jost-s added 3 commits January 10, 2024 15:48

chore: revert host crate version bump

b1e5afa

test: fix concurrent calls

fb54445

chore: add cargo lock

830dee7

ThetaSinner approved these changes Jan 10, 2024

View reviewed changes

jost-s added 3 commits January 11, 2024 10:48

revert some formatting

00ac4ab

comment bench

0047757

update docs

307fe6a

jost-s added 2 commits January 11, 2024 11:53

fix: use cache's runtime engine

5183d21

bring back ios workaround

e22e562

thedavidmeister requested changes Jan 16, 2024

View reviewed changes

test: put module test behind test cfg

80c7c24

thedavidmeister approved these changes Jan 16, 2024

View reviewed changes

jost-s merged commit c22964a into develop Jan 16, 2024
5 checks passed

jost-s deleted the refactor/reinstate-module-cache branch January 16, 2024 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: reinstate module cache #106

refactor: reinstate module cache #106

jost-s commented Jan 5, 2024 •

edited

Loading

jost-s Jan 10, 2024

jost-s Jan 10, 2024

jost-s Jan 10, 2024

ThetaSinner Jan 10, 2024

jost-s Jan 10, 2024 •

edited

Loading

ThetaSinner Jan 10, 2024

ThetaSinner left a comment

ThetaSinner Jan 10, 2024

ThetaSinner Jan 10, 2024

jost-s Jan 11, 2024

ThetaSinner Jan 11, 2024

jost-s Jan 11, 2024 •

edited

Loading

ThetaSinner Jan 10, 2024

ThetaSinner Jan 10, 2024

ThetaSinner Jan 10, 2024

jost-s Jan 11, 2024

ThetaSinner Jan 10, 2024

ThetaSinner Jan 10, 2024

jost-s Jan 11, 2024

ThetaSinner Jan 11, 2024

thedavidmeister Jan 16, 2024

jost-s Jan 16, 2024

thedavidmeister Jan 16, 2024

jost-s Jan 16, 2024

ThetaSinner Jan 10, 2024

thedavidmeister left a comment

thedavidmeister Jan 16, 2024

refactor: reinstate module cache #106

refactor: reinstate module cache #106

Conversation

jost-s commented Jan 5, 2024 • 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

jost-s Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThetaSinner 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

jost-s Jan 11, 2024 • 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

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

thedavidmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jost-s commented Jan 5, 2024 •

edited

Loading

jost-s Jan 10, 2024 •

edited

Loading

jost-s Jan 11, 2024 •

edited

Loading