-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
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 So, by this
you also mean |
Somehow we're going to want to be able to support threaded wasm where instances are connected to each other via a 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. |
@alexcrichton what about doing some form of garbage collection pass? |
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. |
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: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 inwasmtime
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 theStore
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 aStore
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 aModule
I wouldn't expect that to be tied to a particular store. AModule
should have its own compiled memory cached, but this can be instantiated into anyStore
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 anInstance
cannot be sent across threads (although this is up for debate), but you almost for sure want to share aModule
across threads. You may even want multiple threads to be part of a singleStore
, but if aStore
has to have a handle to all of itsInstance
objects then this is sort of a backwards relation becauseStore
needs to be threadsafe, but it can't be threadsafe becauseInstance
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
andEngine
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.The text was updated successfully, but these errors were encountered: