Upgrade to Wasmer Reborn (part 1) #504

webmaster128 · 2020-08-05T12:32:03Z

Closes #503
Closes #462
Closes #501
Closes #375
Closes #495
Closes #555

ethanfrey

Impressive PR.

I wrote a lot of questions that came up trying to understand the new APIs and your usage of them better. However, the code looks solid. Happy for another pair of eyes on this, but from my end, I see nothing blocking merging this.

Also, very curious as to how the benchmarks change when this is merged. Would be great to have a before/after diagram (or some tracking of it)

ethanfrey · 2020-12-10T09:49:54Z

contracts/burner/Cargo.toml

@@ -22,12 +22,13 @@ incremental = false
 overflow-checks = true

 [features]
+# Change this to [] if you don't need Windows support and want faster integration tests.


This only affects the tests, right?
Meaning, cargo test works out of the box on all platforms, but linux/osx can opt into cargo test --no-default-features for speed

The alternative would be default [], cargo test does the right thing on osx/linux, and you would need cargo test --features cranelift to run on windows at all.

I am leaning towards the second as a pattern. But it would be cool to see if that could be done automatically for both.

Hmmm.. wasted some time reflecting on Cargo files.

There is eg:

[target.x86_64-pc-windows-gnu.dependencies] byte = "0.2.4"

But I cannot find the equivalent for enabling target-specific feature flags.

Minor point, but it would good to set a proper template for all the contracts

This is only relevant for integration tests. Unit tests are not affected.

The alternative would be default [], cargo test does the right thing on osx/linux, and you would need cargo test --features cranelift to run on windows at all.

Jupp, but it also requires different documentation. I went for the less optimized but more beginner friendly way. But open for change.

Hmmm.. wasted some time reflecting on Cargo files.

See also #649, which is probably what you are looking for

packages/vm/src/cache.rs

ethanfrey · 2020-12-10T10:08:35Z

packages/vm/src/cache.rs

@@ -125,7 +126,7 @@ where
        }

        // Get module from file system cache
-        if let Some(module) = self.fs_cache.load(checksum)? {
+        if let Some(module) = self.fs_cache.load(checksum, options.memory_limit)? {


This take an optional config memory limit (very nice), why not config above?

This limit is not optional but required for every instanciation.
You can have a single cache and get instances with different memory limits out of it. Not that we need it but I ran into design trouble when I had a fixed limit in the cache.

ethanfrey · 2020-12-10T10:10:10Z

packages/vm/src/cache.rs

@@ -476,7 +479,7 @@ mod test {
    }

    #[test]
-    #[cfg(feature = "default-singlepass")]
+    #[cfg(feature = "metering")]


Okay, so this code is just left as unmigrated placeholders and fails if metering flag is enabled now?
(Which is good to push into a separate PR)

Jupp, at some point I realized that adding a simple metering flag makes almost everything compile and pass.

packages/vm/src/calls.rs

ethanfrey · 2020-12-10T10:48:54Z

packages/vm/src/wasm_backend/gas.rs

@@ -0,0 +1,170 @@
+use crate::backend::{Querier, Storage};


This whole file is a TODO/placeholder, right? (I know you said metering was still pending upstream)

Right, lots of noop and outdated code here

ethanfrey · 2020-12-10T10:51:19Z

packages/vm/src/wasm_backend/limiting_tunables.rs

+    ///
+    /// Delegated to base.
+    fn memory_style(&self, memory: &MemoryType) -> MemoryStyle {
+        let adjusted = self.adjust_memory(memory);


no need to validate here?

We validate everything we need to when storing the code the first time. Then self.adjust_memory just sets the maximum for the memory.

Ah, now I know what you mean.

The result type is fixed by the Tunables trait in Wasmer and I can't return errors. But everything that goes through here also calls one of the other functions.

ethanfrey · 2020-12-10T10:52:03Z

packages/vm/src/wasm_backend/limiting_tunables.rs

+        Self { limit, base }
+    }
+
+    /// Takes in input memory type as requested by the guest and sets


Great comments and clear logic here

ethanfrey · 2020-12-10T10:53:42Z

packages/vm/src/wasm_backend/limiting_tunables.rs

+    }
+}
+
+impl<T: Tunables> Tunables for LimitingTunables<T> {


I am unclear from here what is called when a new page is allocated in an existing dynamic memory.

Or no need to intercept that, as we ensure valid minimum/maximum when they create the memory table in the first place, so the internal logic can handle dynamic page allocation?

Also, no intercept on pages means we cannot charge gas per memory usage, right? (No need to do so, I just remember the idea was floating around and curious if possible with the current API)

Right, we set a maximum once and Wasmer checks that the number of pages does not exceed the maximum internally.

Also, no intercept on pages means we cannot charge gas per memory usage, right?

Basically yes.

In the new metering we could implement special treatment for the memory grop op code. But then we also need to look at the minimum. So if we really really want, it can be done. But I'd say memory usage if free within the allowed range.

ethanfrey · 2020-12-10T10:54:53Z

packages/vm/src/wasm_backend/store.rs

+}
+
+/// Created a store with no compiler and the given memory limit (in pages)
+pub fn make_store_headless(memory_limit: Size) -> Store {


Ah, headless can run pre-compiled code, but not compile itself, which makes it lighter to start up when running many instances, right?

Yes. This is also usefull for cross compilation, e.g. for IoT stuff. You compile one machine and run in a much more limited environment.

webmaster128 added the WIP work in progress label Aug 5, 2020

webmaster128 changed the title ~~Upgrade to wasmer reborn~~ Upgrade to Wasmer Reborn Aug 5, 2020

webmaster128 force-pushed the upgrade-to-wasmer-reborn branch from 7eb1351 to 8685586 Compare September 8, 2020 08:04

webmaster128 added 6 commits September 10, 2020 09:28

Update Wasmer dependencies

d0bfe86

Disable middleware

5aef528

Adapt code to new API

de354aa

Update to latest master of Wasmer

94102a7

Update Wasmer error handling

cadafc5

Make VM compile (no iterator or tests)

cc94242

webmaster128 force-pushed the upgrade-to-wasmer-reborn branch from 3274902 to cc94242 Compare September 14, 2020 12:01

webmaster128 added 2 commits September 15, 2020 17:45

Replace try_into! with ?

020ad3b

Update Lockfile to latest Wasmer master

b4805cc

ethanfrey mentioned this pull request Oct 1, 2020

Investigate alternative wasm runtimes #555

Closed

webmaster128 added 17 commits October 6, 2020 15:19

Merge branch 'master' into upgrade-to-wasmer-reborn

c92ed36

Merge branch 'master' into upgrade-to-wasmer-reborn

88f2053

Add FfiError::IteratorDoesNotExist

cd9d05b

Replace range with scan/next in Storage trait

ea96f77

Update MockStorage to new trait

786719f

Use with_storage_from_context in do_next

fad1b70

Remove with_iterator_from_context

8605e14

Remove VmError::IteratorDoesNotExist

ae4cc8b

Remove iterators from ContextData

069a6a5

Implement AddAssign for GasInfo

0bf8d4d

Handle errors in self.next perperly

1c648c7

Adapt burner integration tests to new storage

b14e190

Add note on iterator IDs

c90020b

Remove StorageIterator and MockIterator

a40eb0e

Add CHANGELOG entries

5fe4cbe

Remove outdated doc comment

30a3e82

Bump wasmer version in lockfile

a709584

webmaster128 added 3 commits December 8, 2020 07:52

Update Wasmer to 9fc32dfd0f3

81d281b

Run all tests in local testing

48730f3

Remove usages of Rust nightly

63e7207

webmaster128 requested a review from maurolacy December 8, 2020 07:19

webmaster128 added 4 commits December 8, 2020 09:07

Remove temporary testing script

1d9dae1

Fix cosmwasm-std version after merge conflict

10fdde0

Update VM's Testing docs

6004449

Avoid double references to Memory

fde0d32

webmaster128 requested a review from ethanfrey December 8, 2020 08:58

webmaster128 mentioned this pull request Dec 9, 2020

Add DeterministicMiddleware to Cranelift setup #311

Closed

webmaster128 added 8 commits December 9, 2020 09:51

Merge branch 'master' into upgrade-to-wasmer-reborn

2cba528

Update Wasmer to e1c92737

0692deb

Merge branch 'master' into upgrade-to-wasmer-reborn

93fe981

Merge branch 'master' into upgrade-to-wasmer-reborn

3c23c6d

Fix job name package_vm_singlepass -> package_vm

707cc88

Set RUST_BACKTRACE in benchmarking job and fix gas issue in CI

3a602a7

Add ref_to_u32

c74d978

Use ref_to_u32 for Wasmer's Value

54dacc9

ethanfrey approved these changes Dec 10, 2020

View reviewed changes

webmaster128 added 7 commits December 10, 2020 15:28

Make memory limit optional to avoid the need for a fake limit

6551573

Use a single immutable env reference

2f8f86d

Streamline callback and return generic types

81381c1

Rename module context -> environment

8f0815d

Rename to move_into_environment/move_out_of_environment

a5194fd

Rename Env -> Environment to avoid conflict with Env in contracts

3f0e17e

Add print_debug to Environment

e657e59

webmaster128 merged commit c44ff51 into master Dec 10, 2020

webmaster128 deleted the upgrade-to-wasmer-reborn branch December 10, 2020 16:20

webmaster128 mentioned this pull request Dec 14, 2020

Upgrade to Wasmer Reborn (part 2) #663

Merged

webmaster128 changed the title ~~Upgrade to Wasmer Reborn~~ Upgrade to Wasmer Reborn (part 1) Dec 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Wasmer Reborn (part 1) #504

Upgrade to Wasmer Reborn (part 1) #504

webmaster128 commented Aug 5, 2020 •

edited

Loading

ethanfrey left a comment

ethanfrey Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

webmaster128 Dec 10, 2020

ethanfrey Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

ethanfrey Dec 10, 2020

webmaster128 Dec 10, 2020

Upgrade to Wasmer Reborn (part 1) #504

Upgrade to Wasmer Reborn (part 1) #504

Conversation

webmaster128 commented Aug 5, 2020 • edited Loading

ethanfrey 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

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

webmaster128 commented Aug 5, 2020 •

edited

Loading