Skip {enter,exit}-sync-call for "thread-transparent" adapters

[fitzgen]

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.

[cfallin]

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?

[alexcrichton]

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?

[fitzgen]

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.

[alexcrichton]

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).

[fitzgen]

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.