Simplify thread-local management of the store#201
Conversation
This commit is a simplification of the management of thread locals used to transfer a store across the `Future::poll` boundary to pass it from the caller to internal futures we're polling. Previously there were two thread locals where a store would migrate between them as appropriate, and these two thread locals were adjacent to the rest of `concurrent.rs` which accessed it from a number of locations. After this commit all TLS accesses are isolated in a single `tls.rs` file which is intended to be managable to audit in its entirety. Both TLS globals have been folded in a single global that is two pointers large, storing a `dyn VMStore` pointer. The store now has internal state associated with the current instance being polled as well which removes th need for some extra state to be passed in these globals. Additionally functions such as `with_local_instance` where two arguments were produced are now split into one argument (the store) comes from TLS and the other (the instance) comes from a closed over argument in futures/etc. The culmination of this is that all TLS management goes through a smaller set of primitives than before which is intended to be easier to audit for unsafe.
| /// values (e.g. futures) and that internally in the runtime we aren't doing | ||
| /// anything "weird" with threads for example. | ||
| pub unsafe trait VMStore { | ||
| pub unsafe trait VMStore: 'static { |
There was a problem hiding this comment.
No big deal, but curious why you decided to add this bound again. Was it unavoidable given the use in TLS?
There was a problem hiding this comment.
The basic problem was this where I wanted to convert from &mut dyn VMStore-as-an-argument to *mut dyn VMStore-stored-somewhere-else. Lifetime inference works differently in these situations so as an argument it's &mut (dyn VMStore + '_) but store in a struct it's *mut (dyn VMStore + 'static) (I think at least). In lieu of sprinkling 'static bounds around I figured it's easiest to just add it here where it's already true because Store<T>: 'static all the time anyway now.
There was a problem hiding this comment.
Makes sense. If we needed to, I suspect we could avoid the bound by using &mut StoreOpaque and *mut StoreOpaque instead, but we don't need to.
Co-authored-by: Joel Dice <joel.dice@fermyon.com>
2 participants
This commit is a simplification of the management of thread locals used to transfer a store across the
Future::pollboundary to pass it from the caller to internal futures we're polling. Previously there were two thread locals where a store would migrate between them as appropriate, and these two thread locals were adjacent to the rest ofconcurrent.rswhich accessed it from a number of locations. After this commit all TLS accesses are isolated in a singletls.rsfile which is intended to be managable to audit in its entirety.Both TLS globals have been folded in a single global that is two pointers large, storing a
dyn VMStorepointer. The store now has internal state associated with the current instance being polled as well which removes th need for some extra state to be passed in these globals. Additionally functions such aswith_local_instancewhere two arguments were produced are now split into one argument (the store) comes from TLS and the other (the instance) comes from a closed over argument in futures/etc. The culmination of this is that all TLS management goes through a smaller set of primitives than before which is intended to be easier to audit for unsafe.