component::__internal::InstanceType now owns its contents #10616
Conversation
Having refs to a pair of arcs made it less expensive to create an InstanceType, but tied its to a borrow of the ComponentInstance (transitively the Store) which makes some new typechecking operations which have an AsContextMut impossible. This has no direct benefit in this PR, but it split out of #10610 because it is more easily reviewed in isolation.
github-actions
bot
added
the
wasmtime:api
| @@ -355,7 +355,7 @@ impl<'a, T> LowerContext<'a, T> { | |||
|
|
|||
| /// Returns the instance type information corresponding to the instance that | |||
| /// this context is lowering into. | |||
| pub fn instance_type(&self) -> InstanceType<'_> { | |||
| pub fn instance_type(&self) -> InstanceType { | |||
There was a problem hiding this comment.
We talked briefly about this on Zulip, but this is the call that I'm worried about perf-wise. This is used during lifting/lowering of ResourceAny (transitively through the resource_type helper method which delegates to this one). I'd prefer to keep the Arc-cloning off the hot path there if possible, but I need to read up more on the other PR for better understanding the motivation behind this one.
There was a problem hiding this comment.
Agreed, thanks for finding where it was in the hot path, I missed that. If for some reason the approach you outlined in #10610 doesn't cash out I will at very least make sure we can amortize the clone to only happen once per store, so it doesn't hurt resource lifting/lowering.
|
Replaced by #10621 |
Having refs to a pair of arcs made it less expensive to create an InstanceType, but tied its to a borrow of the ComponentInstance (transitively the Store) which makes some new typechecking operations which have an AsContextMut impossible.
This has no direct benefit in this PR, but it split out of #10610 because it is more easily reviewed in isolation.