Linker::instantiate_pre could avoid mutating a Store
#5675
Comments
|
I was just looking at a wasmtime embedder that constructs an otherwise-unused |
|
This isn't a very helpful answer, but I wanted this as well when we introduced instantiate_pre, and there was some complexity of the implementation that made it extremely difficult, so I gave up. The situation may have changed and I'd welcome if someone was able to do it! |
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this issue
Feb 1, 2023
This commit relaxes a requirement of the `InstancePre` API, notably its construction via `Linker::instantiate_pre`. Previously this function required a `Store<T>` to be present to be able to perform type-checking on the contents of the linker, and now this requirement has been removed. Items stored within a linker are either a `HostFunc`, which has type information inside of it, or an `Extern`, which doesn't have type information inside of it. Due to the usage of `Extern` this is why a `Store` was required during the `InstancePre` construction process, it's used to extract the type of an `Extern`. This commit implements a solution where the type information of an `Extern` is stored alongside the `Extern` itself, meaning that the `InstancePre` construction process no longer requires a `Store<T>`. One caveat of this implementation is that some items, such as tables and memories, technically have a "dynamic type" where during type checking their current size is consulted to match against the minimum size required of an import. This no longer works when using `Linker::instantiate_pre` as the current size used is the one when it was inserted into the linker rather than the one available at instantiation time. It's hoped, however, that this is a relatively esoteric use case that doesn't impact many real-world users. Additionally note that this is an API-breaking change. Not only is the `Store` argument removed from `Linker::instantiate_pre`, but some other methods such as `Linker::define` grew a `Store` argument as the type needs to be extracted when an item is inserted into a linker. Closes bytecodealliance#5675
|
I was struck with inspiration when thinking about this again and created #5683 |
alexcrichton
added a commit
that referenced
this issue
Feb 2, 2023
* Remove the need to have a `Store` for an `InstancePre` This commit relaxes a requirement of the `InstancePre` API, notably its construction via `Linker::instantiate_pre`. Previously this function required a `Store<T>` to be present to be able to perform type-checking on the contents of the linker, and now this requirement has been removed. Items stored within a linker are either a `HostFunc`, which has type information inside of it, or an `Extern`, which doesn't have type information inside of it. Due to the usage of `Extern` this is why a `Store` was required during the `InstancePre` construction process, it's used to extract the type of an `Extern`. This commit implements a solution where the type information of an `Extern` is stored alongside the `Extern` itself, meaning that the `InstancePre` construction process no longer requires a `Store<T>`. One caveat of this implementation is that some items, such as tables and memories, technically have a "dynamic type" where during type checking their current size is consulted to match against the minimum size required of an import. This no longer works when using `Linker::instantiate_pre` as the current size used is the one when it was inserted into the linker rather than the one available at instantiation time. It's hoped, however, that this is a relatively esoteric use case that doesn't impact many real-world users. Additionally note that this is an API-breaking change. Not only is the `Store` argument removed from `Linker::instantiate_pre`, but some other methods such as `Linker::define` grew a `Store` argument as the type needs to be extracted when an item is inserted into a linker. Closes #5675 * Fix the C API * Fix benchmark compilation * Add C API docs * Update crates/wasmtime/src/linker.rs Co-authored-by: Andrew Brown <[email protected]> --------- Co-authored-by: Andrew Brown <[email protected]>
abrown
added a commit
to abrown/wasmtime
that referenced
this issue
Feb 7, 2023
Due to bytecodealliance#5675, we could not perform an early check whether the module passed to `wasi-threads` could indeed be instantiated. Now that that issue is resolved, this change performs the instantiation check (via `Linker::instantiate_pre`) during construction.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #5484, it would have been ideal to check if an instance was "instantiatable" by a
Linkermuch sooner than when we currently check it (at startup instead of when a thread is spawned). Because aStoreis not available until the thread is spawned andLinker::instantiate_prerequires a mutableStore, we cannot do this check sooner. IfLinker::instantiate_prewere refactored to be called without aStoreor if some other method were available to "check that all the imports can be satisfied," then the check in wasi-threads could be moved up and this issue closed.The text was updated successfully, but these errors were encountered: