Stream: git-wasmtime

Topic: wasmtime / Issue #960 Stores cannot deallocate memory unt...

Wasmtime GitHub notifications bot (Mar 04 2020 at 15:03):

alexcrichton labeled Issue #960:

From our discussion today on the wasmtime call the topic of GC, table.set, and https://github.com/bytecodealliance/wasmtime/issues/954 all came up. It was concluded that today with the MVP you can actually create a cyclic dependency between modules, for example:

(module $a
  (table (export "table") 2 2 funcref)
  (func
    i32.const 1
    call_indirect)
  (elem (i32.const 0) func 0))

(module $b
  (import "a" "table" (table 2 2 funcref))
  (func
    i32.const 0
    call_indirect)
  (elem (i32.const 1) func 0))

These two modules only use MVP features but they have a shared table which cyclically references the two modules.

Our conclusion from this was that Store is actually a "clique" of instances together, and once you instantiate something you can never deallocate anything until all the instances have gone. Internally this would entail ensuring that all types exported in wasmtime have some sort of handle on their store (basically all of them already do, this isn't hard).

Implementation-wise I don't think this will be hard to do, but this does have some critical implications I'd like to sort out first:

• We need to acknowledge that an Instance will never be fully deallocated until all objects referencing the Store have gone away. This feels like a pretty bad crutch to lean on (but it's not like we have much choice) and is something we would need to document thoroughly. You would basically never want user code generating instances into a Store because it means you could OOM quickly. Instead your instance-creation patterns must be known statically. (this may eventually be different with the whole linking proposal, but this would be the status quo that we would have to document)

• If we start thinking of Store as a "clique of instances" then I don't think the API is quite aligned for that today. For example if you compile a Module I wouldn't expect that to be tied to a particular store. A Module should have its own compiled memory cached, but this can be instantiated into any Store object since it's just adding new references to the compiled memory in multiple stores (possibly). Furthermore I don't know how to rationalize the thread-safety here. For example an Instance cannot be sent across threads (although this is up for debate), but you almost for sure want to share a Module across threads. You may even want multiple threads to be part of a single Store, but if a Store has to have a handle to all of its Instance objects then this is sort of a backwards relation because Store needs to be threadsafe, but it can't be threadsafe because Instance can't be threadsafe.

I think a bunch of this may just boil down to "Alex needs further clarification of what's already there", but I don't really see how Store and Engine map to concepts of what you would want today. I understand they came from v8, but I'm not sure why we want them in our API as well and how it maps to thread safety and such. This is all stuff we need to sort out, though, because the current way we treat multiple stores and instances referencing each other is not memory safe and can easily segfault.

Wasmtime GitHub notifications bot (Mar 16 2020 at 15:57):

pepyakin commented on Issue #960:

I think it came from wasm-c-api (which seems to be heavily influenced by v8). It seems that wasm-c-api was inspired by the wasm spec, but in contrast to it, wasm-c-api's Module requires Store for some reason. Looking at their impl, their store is thread-safe in the sense that it is Send but not thread-safe in the sense that multiple threads can use it at the same time.

So, by this

You may even want multiple threads to be part of a single Store

you also mean Send?

Wasmtime GitHub notifications bot (Mar 17 2020 at 18:53):

alexcrichton commented on Issue #960:

Somehow we're going to want to be able to support threaded wasm where instances are connected to each other via a Memory and they can all instantiate the same Module on multiple threads. That's sort of the bare minimum required, but today we've painted ourselves a bit into a corner where Module stores a Store which is not thread safe inherently because Instance fields should never be dropped until a Store is dropped. Furthermore we also require that all imports come from the same Store, which would cause more issues if we were to try to make targeted fixes.

We're basically in a corner right now which I dont think makes sense. We need to figure out, probably from scratch, what we want our multithreading story to be. For example what structures are supposed to be shared, what's the idioms we are expecting for multithreaded instances, etc.

Wasmtime GitHub notifications bot (Apr 29 2020 at 17:47):

alexcrichton closed Issue #960:

From our discussion today on the wasmtime call the topic of GC, table.set, and https://github.com/bytecodealliance/wasmtime/issues/954 all came up. It was concluded that today with the MVP you can actually create a cyclic dependency between modules, for example:

(module $a
  (table (export "table") 2 2 funcref)
  (func
    i32.const 1
    call_indirect)
  (elem (i32.const 0) func 0))

(module $b
  (import "a" "table" (table 2 2 funcref))
  (func
    i32.const 0
    call_indirect)
  (elem (i32.const 1) func 0))

These two modules only use MVP features but they have a shared table which cyclically references the two modules.

Our conclusion from this was that Store is actually a "clique" of instances together, and once you instantiate something you can never deallocate anything until all the instances have gone. Internally this would entail ensuring that all types exported in wasmtime have some sort of handle on their store (basically all of them already do, this isn't hard).

Implementation-wise I don't think this will be hard to do, but this does have some critical implications I'd like to sort out first:

• We need to acknowledge that an Instance will never be fully deallocated until all objects referencing the Store have gone away. This feels like a pretty bad crutch to lean on (but it's not like we have much choice) and is something we would need to document thoroughly. You would basically never want user code generating instances into a Store because it means you could OOM quickly. Instead your instance-creation patterns must be known statically. (this may eventually be different with the whole linking proposal, but this would be the status quo that we would have to document)

• If we start thinking of Store as a "clique of instances" then I don't think the API is quite aligned for that today. For example if you compile a Module I wouldn't expect that to be tied to a particular store. A Module should have its own compiled memory cached, but this can be instantiated into any Store object since it's just adding new references to the compiled memory in multiple stores (possibly). Furthermore I don't know how to rationalize the thread-safety here. For example an Instance cannot be sent across threads (although this is up for debate), but you almost for sure want to share a Module across threads. You may even want multiple threads to be part of a single Store, but if a Store has to have a handle to all of its Instance objects then this is sort of a backwards relation because Store needs to be threadsafe, but it can't be threadsafe because Instance can't be threadsafe.

I think a bunch of this may just boil down to "Alex needs further clarification of what's already there", but I don't really see how Store and Engine map to concepts of what you would want today. I understand they came from v8, but I'm not sure why we want them in our API as well and how it maps to thread safety and such. This is all stuff we need to sort out, though, because the current way we treat multiple stores and instances referencing each other is not memory safe and can easily segfault.

Wasmtime GitHub notifications bot (May 08 2020 at 15:24):

DemiMarie-parity commented on Issue #960:

@alexcrichton what about doing some form of garbage collection pass?

Wasmtime GitHub notifications bot (May 08 2020 at 16:06):

alexcrichton commented on Issue #960:

Indeed that should be able to help clear out memory sooner! That's a pretty major feature though and one we haven't planned on adding any time soon (AFAIK) to wasmtime, so it's aways out if at all.

Last updated: Nov 22 2024 at 17:03 UTC