wasmtime / PR #14270 Skip `{enter,exit}-sync-call` for "t... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #14270 Skip `{enter,exit}-sync-call` for "t...

Wasmtime GitHub notifications bot (Sep 02 2026 at 19:54):

fitzgen opened PR #14270 from fitzgen:sync-adapter-optimizations to bytecodealliance:main:

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.

Wasmtime GitHub notifications bot (Sep 02 2026 at 19:54):

fitzgen requested cfallin for a review on PR #14270.

Wasmtime GitHub notifications bot (Sep 02 2026 at 19:54):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #14270.

Wasmtime GitHub notifications bot (Sep 02 2026 at 19:54):

fitzgen requested wasmtime-core-reviewers for a review on PR #14270.

Wasmtime GitHub notifications bot (Sep 02 2026 at 21:44):

github-actions[bot] added the label wasmtime:api on PR #14270.

Wasmtime GitHub notifications bot (Sep 02 2026 at 22:32):

fitzgen updated PR #14270.

Wasmtime GitHub notifications bot (Sep 03 2026 at 16:01):

:memo: cfallin submitted PR review:

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?

Wasmtime GitHub notifications bot (Sep 03 2026 at 16:01):

:speech_balloon: cfallin created PR review 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?

Wasmtime GitHub notifications bot (Sep 03 2026 at 16:01):

:speech_balloon: cfallin created PR review 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?

Wasmtime GitHub notifications bot (Sep 03 2026 at 16:01):

:speech_balloon: cfallin created PR review 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_instance as 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_transparent to query the result during codegen.

Basically, I want a "stratification" of the API according to its expected usage pattern.

Wasmtime GitHub notifications bot (Sep 04 2026 at 18:31):

fitzgen requested alexcrichton for a review on PR #14270.

Wasmtime GitHub notifications bot (Sep 08 2026 at 18:23):

:memo: alexcrichton submitted PR review:

Overall this seems generally correct to me, but I'm left with a general feeling that the inlining pass isn't the best place to handle this. One example of my unease is that ComponentInstanceDef::Intrinsics is claimed as "yes, this is transparent", but that's not actually true for the context get/set intrinsics. This is later handled in func_def_is_transparent where those are "no, this is not transparent", so I don't think there's a bug here, but I'm also not certain of that.

I've been studying this and reading over old code again, and would it be possible to migrate this analysis to the dfg.rs data structures? Those I think are generally structured exactly as you'd want for this, and this analysis could be a function of ComponentDfg to EntitySet<AdapterId> for example (or, possibly, put a Option<bool> on struct Adapter and fill it in in this analysis pass and then the Option<bool> is unwrapped during adapter generation). This sort of migration would require a relatively major restructuring of thread_transparency.rs, however, so I'm not certain this is the right thing to do.

What I'm roughly thinking is that there'd sets of things that are transparent and they'd be filled in by walking over the order of things in ComponentDfg. For example an InstanceId is transparent if all its arguments are, and an OptionsId would be transparent if everything it points to is additionally transparent. I think this should be a relatively natural analysis to write in a similar manner to other parts of dfg.rs, basically an AST-walk of sorts of the data structures and building up a predicate.

The benefits of this approach would be to avoid complicating the inlining pass more and not having to worry about abstractions like aliases and such. Instead, in theory, everything would be able to process the exact definition something has, for example not having to deal with IntrinsicsImport as well and only dealing with "is this exact unsafe intrinsic transparent".

Does that sound ok to you to rearchitect this? Or do you feel that this pass as-is is the right location to do this?

Unrelatedly, I'd ideally like to review the tests being added here, but ~4kloc of tests for this feature feels kind of intense and I wouldn't really know where to start. I suspect most of them are LLM-generated, but have you had a chance to review the tests themselves? Do you know if it'd be possible to reduce the size of testing without reducing test coverage?

Wasmtime GitHub notifications bot (Sep 08 2026 at 20:12):

fitzgen commented on PR #14270:

One example of my unease is that ComponentInstanceDef::Intrinsics is claimed as "yes, this is transparent", but that's not actually true for the context get/set intrinsics. This is later handled in func_def_is_transparent where those are "no, this is not transparent", so I don't think there's a bug here, but I'm also not certain of that.

The Wasmtime intrinsics instance is transparent because our unsafe intrinsics instance cannot access the task state. The context get/set canon intrinsics aren't actually exported from the unsafe intrinsics instance, and are only implemented as pseudo unsafe intrinsics because we shoe-horned them into that enum rather than spending the time to define their own enum and dealing with the type shepherding fallout (and we should really go back and fix that...)

I've been studying this and reading over old code again, and would it be possible to migrate this analysis to the dfg.rs data structures? Those I think are generally structured exactly as you'd want for this, and this analysis could be a function of ComponentDfg to EntitySet<AdapterId> for example (or, possibly, put a Option<bool> on struct Adapter and fill it in in this analysis pass and then the Option<bool> is unwrapped during adapter generation). This sort of migration would require a relatively major restructuring of thread_transparency.rs, however, so I'm not certain this is the right thing to do.

What I'm roughly thinking is that there'd sets of things that are transparent and they'd be filled in by walking over the order of things in ComponentDfg. For example an InstanceId is transparent if all its arguments are, and an OptionsId would be transparent if everything it points to is additionally transparent. I think this should be a relatively natural analysis to write in a similar manner to other parts of dfg.rs, basically an AST-walk of sorts of the data structures and building up a predicate.

The benefits of this approach would be to avoid complicating the inlining pass more and not having to worry about abstractions like aliases and such. Instead, in theory, everything would be able to process the exact definition something has, for example not having to deal with IntrinsicsImport as well and only dealing with "is this exact unsafe intrinsic transparent".

Does that sound ok to you to rearchitect this? Or do you feel that this pass as-is is the right location to do this?

I can look into this, not married to the current implementation.

Unrelatedly, I'd ideally like to review the tests being added here, but ~4kloc of tests for this feature feels kind of intense and I wouldn't really know where to start. I suspect most of them are LLM-generated, but have you had a chance to review the tests themselves? Do you know if it'd be possible to reduce the size of testing without reducing test coverage?

I did review everything, and probably made things worse by pushing to avoid unit #[test]s because that required building a bunch of internal data structures by hand that are normally only created by translation which I felt was worse (and also verbose but additionally fairly unreadable), but perhaps I can do better by making some stuff dependency injection ish so that we can write them more concisely. Will look into it.

Wasmtime GitHub notifications bot (Sep 08 2026 at 20:21):

alexcrichton commented on PR #14270:

Oh right yeah, good point about ComponentInstanceDef::Intrinsics. My hope is that with the dfg-based approach it'll sort of naturally fall out and we won't even have to consider this, but that's TBD.

For tests ok makes sense, and yeah I'd prefer duplication in *.wast tests over adding new tests/all/*.rs tests (as that way we can hopefully share with other runtimes one day).

Wasmtime GitHub notifications bot (Sep 08 2026 at 22:32):

fitzgen updated PR #14270.

Wasmtime GitHub notifications bot (Sep 08 2026 at 22:33):

fitzgen requested alexcrichton for a review on PR #14270.

Wasmtime GitHub notifications bot (Sep 08 2026 at 22:34):

fitzgen commented on PR #14270:

Factored things out a bit to have its own core "vocabulary" so that it is easily unit testable and made it ultimately a fn(&ComponentDfg) -> EntitySet<AdapterId> and I think things are indeed much cleaner now.

Wasmtime GitHub notifications bot (Sep 09 2026 at 04:23):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 04:23):

:speech_balloon: alexcrichton created PR review comment:

I think everything in module, except transparent_adapters, can drop the pub now? (should help future dead-code lints and such)

Wasmtime GitHub notifications bot (Sep 09 2026 at 04:23):

:speech_balloon: alexcrichton created PR review comment:

One possible option here, if you feel so inclined, to not pass a bool parameter would be to pass in the whole &component and *adapte and have the method internally pluck out these fields.

Wasmtime GitHub notifications bot (Sep 09 2026 at 04:23):

:speech_balloon: alexcrichton created PR review comment:

Huh this seems like we should probably unify the type for adapter and normal options... Anyway for a future PR

Wasmtime GitHub notifications bot (Sep 09 2026 at 04:23):

:speech_balloon: alexcrichton created PR review comment:

Naively I'd expect this test to fail because if something can end up going out to the host I'd expect it transitively to be considered opaque. Is this something where the analysis could change the order of how it processes things or inherit instance-to-instance to detect what's opaque?

If this test fails, I also don't think that would de-optimize the use case you're thinking of, right? For compile-time builtins those components generally won't actually import anything from the host I'd expect.

Wasmtime GitHub notifications bot (Sep 09 2026 at 04:23):

:speech_balloon: alexcrichton created PR review comment:

I'm a bit surprised by these, all of these I'd expect would deal with thread/task state. These are all synthesized by fact modules though so I could also imagine that they don't show up at all since the fact modules haven't been translated yet.

Perhaps these could be unreachable!() and/or move to above?

Wasmtime GitHub notifications bot (Sep 09 2026 at 14:56):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 14:56):

:speech_balloon: fitzgen created PR review comment:

For compile-time builtins those components generally won't actually import anything from the host I'd expect.

Actually, I would expect compile-time builtins to import things from the host because for everything except the simplest getters, there would be a fast/simple inline path and then a slow path that calls into the host in order to e.g. grow a buffer or whatever that is too complicated to do inline.

It seems unfortunate to make things transitive when it isn't necessary for correctness and would only mean skipping optimizations.

Wasmtime GitHub notifications bot (Sep 09 2026 at 15:02):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 15:02):

:speech_balloon: alexcrichton created PR review comment:

Ah ok, in that case yeah that seems fine, but how are you thinking that would look like? Right now wouldn't a compile-time-builtin component importing something from the host poison the entire compile-time-import's component instance to be opaque? you'd have to import something from some other component which then imports from the host I'd imagine

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

fitzgen updated PR #14270.

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

:speech_balloon: fitzgen created PR review comment:

Done

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

:speech_balloon: fitzgen created PR review comment:

Done

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 16:22):

:speech_balloon: fitzgen created PR review comment:

Done

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:17):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:17):

:speech_balloon: fitzgen created PR review comment:

Yeah, it would require keeping the host calls in their own component that re-exports that imported host functions, so that the thread state save/restore is delayed until crossing that component's boundary. I think this should be fine though.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:28):

:thumbs_up: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:28):

alexcrichton added PR #14270 Skip {enter,exit}-sync-call for "thread-transparent" adapters to the merge queue.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:54):

:check: alexcrichton merged PR #14270.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:54):

alexcrichton removed PR #14270 Skip {enter,exit}-sync-call for "thread-transparent" adapters from the merge queue.

Wasmtime GitHub notifications bot (Sep 09 2026 at 19:14):

fitzgen commented on PR #14270:

Thanks for the review!

Last updated: Sep 20 2026 at 18:08 UTC