Add Table example; fix local Table and Memory metadata ownership #1687

MarkMcCaskey · 2020-10-07T23:07:18Z

Adds an example using the Table API.

Work in progress, debugging an issue with growing using the Table API...

Things I'd like to have before shipping:

Code / comments clear enough that what's happening is fully understandable with no extra context
Test for dynamic memory (I don't know how to test this actually works with dynamic memory without editing code, this bug manifests most strongly with dynamic memory -- it may be possible to test memory without it though) - deferred, see Add test for dynamic memory #1769

Review

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

Hywan · 2020-10-08T07:47:34Z

Thanks for working on this.

Hywan

Please don't forget to update examples/README.md.

This commit contains a ton of debug code that we'll need to remove

MarkMcCaskey · 2020-10-14T23:31:38Z

bors try

bors · 2020-10-15T00:05:05Z

try

Build succeeded:

lib/engine/src/tunables.rs

lib/vm/src/memory.rs

syrusakbary · 2020-10-15T20:53:49Z

lib/vm/src/memory.rs

+#[derive(Debug)]
+enum VMMemoryDefinitionOwnership {
+    /// The `VMMemoryDefinition` is owned by the `Instance` and we should use
+    /// its memory. This is how a local memory that's exported should be stored.
+    VMOwned(NonNull<VMMemoryDefinition>),
+    /// The `VMMemoryDefinition` is owned by the host and we should manage its
+    /// memory. This is how an imported memory that doesn't come from another
+    /// Wasm module should be stored.
+    HostOwned(Box<UnsafeCell<VMMemoryDefinition>>),
+}


I'm not super fan of this division. A memory is a memory, regardless of who owns it.

Yeah, this is encapsulated internally and the reason we store it this way is so that freeing memory works correctly:

VMMemoryDefinitionOwnership::VMOwned won't free the memory when it gets dropped but VMMemoryDefinitionOwnership::HostOwned will.

I agree that it might make sense to define methods on this type to avoid having to match in the implementation code.

Let me know if that's not what you had in mind.

Hypothetically if we don't fix it this way, then here's another thing we could do, not that it wouldn't be a ton of work. Make sure that VM* objects exist only inside the VMContext/Instance. wasmer_api:: would need two objects for memory, one that would be a handle to any kind of memory and the other which would be a host memory (detached from instance/vmcontext) and would contain the backing store. The VMMemoryImport would no longer point to a VMMemoryDefinition because a host Memory wouldn't have one, it would have to point to all the relevant parts (the backing, the enough to call grow and size, etc.) directly. The user has to keep the host memory and backing store alive, probably by Rc or Arc, and we could somewhat fold it into the handle (api::Memory) with an enum that indicates whether it's holding a whole backing store, or pointing to a VMMemoryDefinition.

The idea being that VMMemoryImport would never own its metadata?

I'm not sure I fully understood what you're suggesting but to make sure we're on the same page:

The generated code always needs a VMMemoryDefinition.

If it's local, it lives in VM-owned memory

if it's imported it lives somewhere else (either another VM's memory or the host)

In the current code, due to Arc, an Instance using an imported host type will ensure that it's never freed while the Instance is using it.

We need an extra layer of indirection for imported VM Objects to make sure that modifications are reflected in the generated code. It's possible to remove this layer of indirection but it means we'd need update logic.

Right, the idea being that VMMemoryImport would never own its metadata. In this model the generated code would need to change to work with a VMMemoryImport.

The compiler-generated code needs to know the base pointer and the size. It also has to be able to grow and size, both of which go through libcalls (even in the local owned memory case). More subtly we need invalidation rules for the base pointer and the size, for instance can we assume they never change at any time the function is executing?

One possible solution. You could have the VMMemoryImport have ptr_to_ versions of the fields the VMMemoryDefinition has. Instead of ptr-to-vmmemorydefinition and arc it would have a ptr_to_base and ptr_to_current_length, then the Arc (or whatever we call the object that keeps the pointed-to-memory alive, if anything) goes in the Instance::memories, not the VMContext objects.

The important part of my suggestion is to start with a rule that VM* objects are only ever part of a VMContext and then follow that through the rest of the design.

The important part of my suggestion is to start with a rule that VM* objects are only ever part of a VMContext and then follow that through the rest of the design.

I think this makes ownership harder in the case of a VM object imported into multiple instances, who is responsible for freeing it? What about memories created when there are no Instances created?

I'm not necessarily opposed to the changes but I think we should land this PR first, this change is self-contained and fixes user-facing bugs; the implementation can easily be changed later in my opinion.

examples/README.md

examples/memory.rs

Hywan · 2020-10-16T18:57:26Z

I did a very quick review, but discussion happens on Slack, easier for the moment.

examples/README.md

examples/memory.rs

lib/api/src/tunables.rs

lib/engine/src/tunables.rs

nlewycky · 2020-10-21T20:40:22Z

lib/vm/src/instance.rs

+        let num_memories = offsets.num_local_memories;
+        let num_memories = usize::try_from(num_memories).unwrap();
+        let mut out = Vec::with_capacity(num_memories);
+        // TODO: better encapsulate this logic, this shouldn't be duplicated


(No action required:) I feel like this is a great example of a TODO that shouldn't have a bug filed.

lib/vm/src/instance.rs

nlewycky · 2020-10-21T21:05:46Z

lib/vm/src/memory.rs

+#[derive(Debug)]
+enum VMMemoryDefinitionOwnership {
+    /// The `VMMemoryDefinition` is owned by the `Instance` and we should use
+    /// its memory. This is how a local memory that's exported should be stored.
+    VMOwned(NonNull<VMMemoryDefinition>),
+    /// The `VMMemoryDefinition` is owned by the host and we should manage its
+    /// memory. This is how an imported memory that doesn't come from another
+    /// Wasm module should be stored.
+    HostOwned(Box<UnsafeCell<VMMemoryDefinition>>),
+}


Hypothetically if we don't fix it this way, then here's another thing we could do, not that it wouldn't be a ton of work. Make sure that VM* objects exist only inside the VMContext/Instance. wasmer_api:: would need two objects for memory, one that would be a handle to any kind of memory and the other which would be a host memory (detached from instance/vmcontext) and would contain the backing store. The VMMemoryImport would no longer point to a VMMemoryDefinition because a host Memory wouldn't have one, it would have to point to all the relevant parts (the backing, the enough to call grow and size, etc.) directly. The user has to keep the host memory and backing store alive, probably by Rc or Arc, and we could somewhat fold it into the handle (api::Memory) with an enum that indicates whether it's holding a whole backing store, or pointing to a VMMemoryDefinition.

lib/vm/src/table.rs

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

MarkMcCaskey · 2020-10-27T19:01:07Z

bors r+

bors · 2020-10-27T19:54:37Z

Build succeeded:

Add Table example

dd62823

MarkMcCaskey requested a review from syrusakbary as a code owner October 7, 2020 23:07

syrusakbary approved these changes Oct 7, 2020

View reviewed changes

Hywan reviewed Oct 8, 2020

View reviewed changes

Fix local memory ownership issue -- WIP

4fc8ace

This commit contains a ton of debug code that we'll need to remove

MarkMcCaskey requested review from losfair and nlewycky as code owners October 14, 2020 22:16

Fix local table ownership issue

49fe173

MarkMcCaskey changed the title ~~Add Table example~~ Add Table example; fix local Table and Memory metadata ownership Oct 14, 2020

Merge branch 'master' into feature/add-table-example

de79a40

bors bot added a commit that referenced this pull request Oct 14, 2020

Try #1687:

fdddd62

MarkMcCaskey requested review from Hywan and removed request for losfair October 15, 2020 00:36

Add table and memory example to examples/README.md

33c32da

syrusakbary reviewed Oct 15, 2020

View reviewed changes

lib/engine/src/tunables.rs Outdated Show resolved Hide resolved

syrusakbary reviewed Oct 15, 2020

View reviewed changes

lib/vm/src/memory.rs Outdated Show resolved Hide resolved

syrusakbary reviewed Oct 15, 2020

View reviewed changes

Split memory, table creation. Mark unsafe functions unsafe

9fdbb08

Hywan reviewed Oct 16, 2020

View reviewed changes

examples/README.md Outdated Show resolved Hide resolved

examples/memory.rs Outdated Show resolved Hide resolved

examples/memory.rs Outdated Show resolved Hide resolved

Abstract over metadata ownership in Memory/Table fn impls

c5b6d9f

nlewycky reviewed Oct 21, 2020

View reviewed changes

Add misc updates from feedback

729a044

MarkMcCaskey mentioned this pull request Oct 21, 2020

Memory example #1749

Closed

Add link to tracking issue in TODO in memory example

d6af684

jubianchi mentioned this pull request Oct 22, 2020

Add more examples for the Rust API #1752

Merged

1 task

MarkMcCaskey mentioned this pull request Oct 23, 2020

Globals are already stored inside the VMContext. #1758

Closed

syrusakbary approved these changes Oct 27, 2020

View reviewed changes

Merge branch 'master' into feature/add-table-example

adc40ae

MarkMcCaskey mentioned this pull request Oct 27, 2020

Add test for dynamic memory #1769

Closed

bors bot merged commit 04091d8 into master Oct 27, 2020

bors bot deleted the feature/add-table-example branch October 27, 2020 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Table example; fix local Table and Memory metadata ownership #1687

Add Table example; fix local Table and Memory metadata ownership #1687

MarkMcCaskey commented Oct 7, 2020 •

edited

Loading

Hywan commented Oct 8, 2020

Hywan left a comment

MarkMcCaskey commented Oct 14, 2020

bors bot commented Oct 15, 2020

syrusakbary Oct 15, 2020

MarkMcCaskey Oct 15, 2020 •

edited

Loading

nlewycky Oct 21, 2020

MarkMcCaskey Oct 21, 2020

nlewycky Oct 21, 2020

MarkMcCaskey Oct 21, 2020

Hywan commented Oct 16, 2020

nlewycky Oct 21, 2020

nlewycky Oct 21, 2020

MarkMcCaskey commented Oct 27, 2020

bors bot commented Oct 27, 2020

Add Table example; fix local Table and Memory metadata ownership #1687

Add Table example; fix local Table and Memory metadata ownership #1687

Conversation

MarkMcCaskey commented Oct 7, 2020 • edited Loading

Review

Hywan commented Oct 8, 2020

Hywan left a comment

Choose a reason for hiding this comment

MarkMcCaskey commented Oct 14, 2020

bors bot commented Oct 15, 2020

try

syrusakbary Oct 15, 2020

Choose a reason for hiding this comment

MarkMcCaskey Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

nlewycky Oct 21, 2020

Choose a reason for hiding this comment

MarkMcCaskey Oct 21, 2020

Choose a reason for hiding this comment

nlewycky Oct 21, 2020

Choose a reason for hiding this comment

MarkMcCaskey Oct 21, 2020

Choose a reason for hiding this comment

Hywan commented Oct 16, 2020

nlewycky Oct 21, 2020

Choose a reason for hiding this comment

nlewycky Oct 21, 2020

Choose a reason for hiding this comment

MarkMcCaskey commented Oct 27, 2020

bors bot commented Oct 27, 2020

MarkMcCaskey commented Oct 7, 2020 •

edited

Loading

MarkMcCaskey Oct 15, 2020 •

edited

Loading