Create example for limiting memory with Tunables #1730

webmaster128 · 2020-10-16T18:48:47Z

Description

This is an attempt for solving #1360 with the new Tunables API. It could be developed further to address #1588 if you like.

I'd appreciate a code review to ensure I'm on the right track.

Open questions:

Is it expected that I had to add wasmer-vm?
Should I really create a new Target for this use case when engine already has one set?
I used BaseTunables for the base implementation and sticked with Tunables for the trait name, because it makes most sense to me. However, in lib/api/src/tunables.rs it is done the other way round. Any thoughts on the naming issue?
The import collision between wamer::Memory and wasmer_vm::Memory is inconvenient. Can it be avoided or do I do it correctly here?

Review

Add a short description of the the change to the CHANGELOG.md file

syrusakbary · 2020-10-16T19:34:58Z

examples/tunables_limit_memory.rs

+use wasmer::{
+    imports, wat2wasm, Instance, Memory, MemoryError, MemoryType, Module, Pages, Store, TableType,
+    Tunables as BaseTunables,
+};
+use wasmer_compiler::Target;
+use wasmer_compiler_cranelift::Cranelift;
+use wasmer_engine::Tunables;
+use wasmer_engine_jit::JIT;
+use wasmer_vm::{Memory as MemoryTrait, MemoryStyle, Table, TableStyle};


For the example, I think it would be better to get some of the structs directly from Wasmer (so people don't depend on other subpackages).

Suggested change

use wasmer::{

imports, wat2wasm, Instance, Memory, MemoryError, MemoryType, Module, Pages, Store, TableType,

Tunables as BaseTunables,

};

use wasmer_compiler::Target;

use wasmer_compiler_cranelift::Cranelift;

use wasmer_engine::Tunables;

use wasmer_engine_jit::JIT;

use wasmer_vm::{Memory as MemoryTrait, MemoryStyle, Table, TableStyle};

use wasmer::{

imports, wat2wasm, Instance, Memory, MemoryError, MemoryType, Module, Pages, Store, TableType,

Tunables, WasmerTunables, Target, Memory, RuntimeMemory, MemoryStyle, RuntimeTable, TableStyle

};

use wasmer_compiler_cranelift::Cranelift;

use wasmer_engine_jit::JIT;

This will imply exposing the following structs in the Wasmer API (lib/api/src/lib.rs):

pub use crate::tunables::Tunables as WasmerTunables; pub use wasmer_engine::Tunables; pub use wasmer_vm::{Memory as RunnableMemory, MemoryStyle, Table as RunnableTable, TableStyle};

Note: other alternative would be exposing the subcrates as submodules off the API:

Suggested change

use wasmer::{

imports, wat2wasm, Instance, Memory, MemoryError, MemoryType, Module, Pages, Store, TableType,

Tunables as BaseTunables,

};

use wasmer_compiler::Target;

use wasmer_compiler_cranelift::Cranelift;

use wasmer_engine::Tunables;

use wasmer_engine_jit::JIT;

use wasmer_vm::{Memory as MemoryTrait, MemoryStyle, Table, TableStyle};

use wasmer::{

imports, wat2wasm, Instance, Memory, MemoryError, MemoryType, Module, Pages, Store, TableType,

Target, Tunables as BaseTunables,

engine::Tunables,

vm::{Memory as MemoryTrait, MemoryStyle, Table, TableStyle};

};

use wasmer_compiler_cranelift::Cranelift;

use wasmer_engine_jit::JIT;

pub mod engine { pub use wasmer_engine::Tunables; } pub mod vm { pub use wasmer_vm::{Memory, MemoryStyle, Table, TableStyle}; }

Thoughts @MarkMcCaskey ?

Yeah I can see either way, for things that conflict in name like Memory namespacing can be a nice way to deal with it, I think expecting people to know about use x as y syntax in Rust is probably not a good assumption (though maybe the error message you get tells you how to fix it), so I'd be more in favor of the public submodule idea than exporting 2 things with the same name at the same scope level.

I think expecting people to know about use x as y syntax in Rust is probably not a good assumption (though maybe the error message you get tells you how to fix it)

Agreed. However, in my case the bigger problem was very very confusing error messages about types that are Sized or size not known at compile time when I had Memory (trait) importet and then used Vec<Memory> in the code, which referes to the exported Memory (struct). Took a while to realize there are more than one of those in Wasmer.

syrusakbary

Thanks a lot for this PR, it pushes things on the right direction.

I just ping'd @MarkMcCaskey so we can see how to handle properly the imports, but other than that, it looks great... good work!

MarkMcCaskey · 2020-10-17T00:12:42Z

examples/tunables_limit_memory.rs

+        if requested.minimum > self.max_memory {
+            return Err(MemoryError::Generic(
+                "Minimum of requested memory exceeds the allowed memory limit".to_string(),
+            ));
+        }
+
+        if let Some(max) = requested.maximum {
+            if max > self.max_memory {
+                return Err(MemoryError::Generic(
+                    "Maximum of requested memory exceeds the allowed memory limit".to_string(),
+                ));
+            }
+        }


One thing, if we expect users to be able to handle these errors programmatically, we should use semantic error values rather than a String via MemoryError::Generic if this error is something fatal then a String is fine

Oh, I just realized that this is an example 😆 , this is neat!

The same still holds though, we may want to add error values for these if we want users to be able to handle them better. We can adjust this this after shipping this example though, so let's ship it as-is for now and then follow up with possibly adding error values that are easier to match on.

I'm happy either way. I could create the error types, but then again the whole concept of memory limits is a customization and the errors also belong in the customization. So maybe the question is whether or not the error type must be customizable in Result<Arc<dyn MemoryTrait>, MemoryError>.

1752: Add more examples for the Rust API r=syrusakbary a=jubianchi # Description The goal is to cover most of the Rust API with examples (and thus tests). Some other PRs already exist for that purpose: * #1730 * #1687 # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: jubianchi <[email protected]>

webmaster128 · 2020-10-27T15:13:47Z

I can update this PR at any time. Just waiting for some clear guidance on the Wasmer exports, especially how to deal with vm and the collisions. This I cannot decide really.

MarkMcCaskey · 2020-10-27T22:45:38Z

@webmaster128 thanks for the ping! I think for now let's go with exporting a sub-module like vm from wasmer, so you can use use wasmer::vm::Memory as _; to get the trait in scope or use it directly as vm::Memory.

I personally think it's probably also a good idea to rename the Memory trait in vm to something unambiguous as well, but we can do that later -- the wasmer sub-module should be a reasonable way to unblock this!

webmaster128 · 2020-10-27T23:44:37Z

Alright, thanks. I now created wasmer::vm, such that I could remove wasmer-vm from the root Cargo.toml again. This is also updated to the changes in the Tunables trait.

The example got a little fancier as the base Tunables is now generic, such that you can use LimitingTunables on top of any Tunables implementation.

MarkMcCaskey

Looks good to me! I'll ping Syrus too

MarkMcCaskey · 2020-10-27T23:47:31Z

lib/api/src/lib.rs

@@ -81,7 +81,17 @@ pub use wasmer_types::{
    Atomically, Bytes, GlobalInit, LocalFunctionIndex, MemoryView, Pages, ValueType,
    WASM_MAX_PAGES, WASM_MIN_PAGES, WASM_PAGE_SIZE,
 };
+
+// TODO: should those be moved into wasmer::vm as well?


Perhaps, but they should probably stay at the top level too!

okay, feel free to just kick out the comment when you are happy with the setup

webmaster128 · 2020-10-27T23:48:12Z

Hope you don't mind I linked this to close #1588. Let me know if I am missing something.

syrusakbary · 2020-10-28T01:32:42Z

Looks great. Merging.

bors r+

bors · 2020-10-28T02:26:31Z

Build succeeded:

webmaster128 requested a review from syrusakbary as a code owner October 16, 2020 18:48

webmaster128 mentioned this pull request Oct 16, 2020

Limit size of linear memory in VM CosmWasm/cosmwasm#462

Closed

syrusakbary reviewed Oct 16, 2020

View reviewed changes

MarkMcCaskey reviewed Oct 17, 2020

View reviewed changes

webmaster128 force-pushed the tunables_limit_memory branch from b13953b to 0773e52 Compare October 17, 2020 08:57

jubianchi mentioned this pull request Oct 22, 2020

Add more examples for the Rust API #1752

Merged

1 task

webmaster128 added 4 commits October 27, 2020 23:55

Create example for limiting memory with Tunables

52cc995

Import Target via wasmer

f00b3b3

Update example to latest Tunables trait

984d478

Make Base tunables generic in LimitingTunables

1337d73

webmaster128 force-pushed the tunables_limit_memory branch from 0773e52 to 1337d73 Compare October 27, 2020 23:40

Remove now unused wasmer-vm from Cargo.toml

9792678

MarkMcCaskey approved these changes Oct 27, 2020

View reviewed changes

syrusakbary approved these changes Oct 28, 2020

View reviewed changes

bors bot merged commit eec90e7 into wasmerio:master Oct 28, 2020

webmaster128 mentioned this pull request Oct 29, 2020

Allow configuration of default memory #1360

Closed

webmaster128 mentioned this pull request Dec 7, 2020

Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} #1894

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create example for limiting memory with Tunables #1730

Create example for limiting memory with Tunables #1730

webmaster128 commented Oct 16, 2020 •

edited

Loading

syrusakbary Oct 16, 2020

MarkMcCaskey Oct 17, 2020

webmaster128 Oct 17, 2020

syrusakbary left a comment

MarkMcCaskey Oct 17, 2020

MarkMcCaskey Oct 17, 2020

webmaster128 Oct 17, 2020 •

edited

Loading

webmaster128 commented Oct 27, 2020

MarkMcCaskey commented Oct 27, 2020

webmaster128 commented Oct 27, 2020

MarkMcCaskey left a comment

MarkMcCaskey Oct 27, 2020

webmaster128 Oct 27, 2020

webmaster128 commented Oct 27, 2020

syrusakbary commented Oct 28, 2020

bors bot commented Oct 28, 2020

Create example for limiting memory with Tunables #1730

Create example for limiting memory with Tunables #1730

Conversation

webmaster128 commented Oct 16, 2020 • edited Loading

Description

Review

syrusakbary Oct 16, 2020

Choose a reason for hiding this comment

MarkMcCaskey Oct 17, 2020

Choose a reason for hiding this comment

webmaster128 Oct 17, 2020

Choose a reason for hiding this comment

syrusakbary left a comment

Choose a reason for hiding this comment

MarkMcCaskey Oct 17, 2020

Choose a reason for hiding this comment

MarkMcCaskey Oct 17, 2020

Choose a reason for hiding this comment

webmaster128 Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

webmaster128 commented Oct 27, 2020

MarkMcCaskey commented Oct 27, 2020

webmaster128 commented Oct 27, 2020

MarkMcCaskey left a comment

Choose a reason for hiding this comment

MarkMcCaskey Oct 27, 2020

Choose a reason for hiding this comment

webmaster128 Oct 27, 2020

Choose a reason for hiding this comment

webmaster128 commented Oct 27, 2020

syrusakbary commented Oct 28, 2020

bors bot commented Oct 28, 2020

webmaster128 commented Oct 16, 2020 •

edited

Loading

webmaster128 Oct 17, 2020 •

edited

Loading