Skip {enter,exit}-sync-call for "thread-transparent" adapters - #14270
Skip {enter,exit}-sync-call for "thread-transparent" adapters#14270fitzgen wants to merge 1 commit into
{enter,exit}-sync-call for "thread-transparent" adapters#14270Conversation
Today, every sync adapter calls `enter-sync-call`, then does its lifting and
lowering of arguments and reesults, and then calls `exit-sync-call`
afterwards. The `{enter,exit}-sync-call` helpers save and restore the old
thread's TLS context and create the new thread's TLS context. For sync-to-sync
calls, we inline these helpers and do their work lazily via the
`VMDeferredThread` machinery. But even so, creating a lazy `VMDeferredThread`
can be pretty expensive if the adapter's callee is just doing like a single load
or store or has been boiled away into returning a constant value.
Therefore, this commit introduces an analysis to find "thread-transparent"
components. These are components that do not `canon lower` any component model
intrinsic to access the thread state, and therefore *cannot* read or write that
state. When we are compiling adapters whose callee is thread-transparent, we
don't even need to `{enter,exit}-sync-call` at all because the callee will not
read/write its thread state, so we don't need to save and restore the current
thread state, we can just leave it in place.
github-actions
Bot
added
the
wasmtime:api
cb770a2 to
3a0c4f0
Compare
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks for this optimization! Some thoughts below.
Overall I will rate my review status as "seems plausible" but I haven't been in this code deeply enough to confidently sign off -- probably @alexcrichton should take a look as well?
| /// Returns whether `adapter` can omit its `{enter,exit}-sync-call` window. | ||
| pub fn adapter_is_transparent(&self, types: &ComponentTypesBuilder, adapter: &Adapter) -> bool { | ||
| Self::signature_is_transparent(types, adapter) | ||
| && self.instances.contains(adapter.lift_options.instance) |
There was a problem hiding this comment.
We query the set here and we both insert and remove from it above -- could we add notes to the API doc-comments specifying the order in which actions must occur for correctness (and a correctness/convergence argument in general)? Something like:
- We initially assume every instance is transparent (that's the
push_instanceas we first walk the instance graph). - Then we scan initializers and remove from the transparent set as we discover the
canon lifts that could observe thread state. - Only after that scan is complete, we can call
adapter_is_transparentto query the result during codegen.
Basically, I want a "stratification" of the API according to its expected usage pattern.
| fn func_def_is_transparent(&self, def: &ComponentFuncDef<'_>) -> bool { | ||
| match def { | ||
| // Goes through an adapter, which saves/restores its own state if | ||
| // needed, but doesn't affect this adapter. |
There was a problem hiding this comment.
This is a key bit to the correctness argument too I think. It's implicit in the name ("transparent") but I think I want to see an explicit statement on the "transitively reached function" problem: naively a sync call to a component that doesn't use the thread-state-observing intrinsics might be fine except that that component calls another component that does. The observation is that the nested call itself will do the state-save if needed. So "transparent" really means that we adopt the thread/task identity of our caller (unobservably) and so doesn't have a transitive nature. Can we write that up somewhere?
| /// Whether this type contains any handle (`own`, `borrow`, `future`, | ||
| /// `stream`, or `error-context`) anywhere within it. Note that this is a | ||
| /// superset of `has_borrow`. | ||
| has_handle: bool, |
There was a problem hiding this comment.
Little nit but these two bools now define an ad-hoc enum with three valid states, not four, because of the implies-relation that you note. Maybe make it an enum with accessors?
2 participants
Today, every sync adapter calls
enter-sync-call, then does its lifting and lowering of arguments and reesults, and then callsexit-sync-callafterwards. The{enter,exit}-sync-callhelpers save and restore the old thread's TLS context and create the new thread's TLS context. For sync-to-sync calls, we inline these helpers and do their work lazily via theVMDeferredThreadmachinery. But even so, creating a lazyVMDeferredThreadcan be pretty expensive if the adapter's callee is just doing like a single load or store or has been boiled away into returning a constant value.Therefore, this commit introduces an analysis to find "thread-transparent" components. These are components that do not
canon lowerany component model intrinsic to access the thread state, and therefore cannot read or write that state. When we are compiling adapters whose callee is thread-transparent, we don't even need to{enter,exit}-sync-callat all because the callee will not read/write its thread state, so we don't need to save and restore the current thread state, we can just leave it in place.