Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stores cannot deallocate memory until all references are gone #960

Closed
alexcrichton opened this issue Feb 20, 2020 · 4 comments · Fixed by #1624
Closed

Stores cannot deallocate memory until all references are gone #960

alexcrichton opened this issue Feb 20, 2020 · 4 comments · Fixed by #1624
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself

Comments

@alexcrichton
Copy link
Member

From our discussion today on the wasmtime call the topic of GC, table.set, and #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.

@pepyakin
Copy link
Contributor

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?

@alexcrichton
Copy link
Member Author

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.

@Demi-Marie
Copy link

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

@alexcrichton
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants