Inline {enter,exit}_sync_call trampolines into sync-to-sync fused adapters#13695
Conversation
github-actions
Bot
added
the
wasmtime:api
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Actually I'll go ahead and flag this for merge. One thing I'd recommend, if you can, is to get an LLM review locally as well to double-check this signs off with no known bugs. If nothing materially changes from my suggestions/comments though seems fine to land.
fitzgen
force-pushed
the
inline-enter-exit-sync-call-in-sync-to-sync-adapters
branch
2 times, most recently
from
00b1e84 to
63b1449
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
fitzgen
force-pushed
the
inline-enter-exit-sync-call-in-sync-to-sync-adapters
branch
from
c0302e3 to
fe5b1e1
Compare
This commit inlines fast paths for the `{enter,exit}_sync_call` component
trampolines in sync-to-sync fused adapters. For cross-component calls of trivial
functions, after inlining, we should avoid all calls in the fused adapter (other
than the `trap` libcall when `may_leave` is set).
The inlining happens during translation, rather than via the regular inliner,
since the lazy thread is stack-allocated within the fused adapter; there would
otherwise be no easy way to reference it.
Fixes bytecodealliance#12311
fitzgen
force-pushed
the
inline-enter-exit-sync-call-in-sync-to-sync-adapters
branch
from
6974e54 to
87c74a8
Compare
946f763
| /// TODO FITZGEN | ||
| #[derive(Clone, Debug)] | ||
| pub enum KnownFunc { | ||
| /// TODO FITZGEN | ||
| FuncKey(FuncKey), | ||
| /// TODO FITZGEN |
There was a problem hiding this comment.
Whoops, thanks for catching this
| // `VMDeferredThread` inside an old, since-unwound stack | ||
| // frame. Therefore, we must reset `current_thread` to avoid | ||
| // potential use-after-free bugs. | ||
| let forced = builder.ins().iconst(pointer_type, 1); |
There was a problem hiding this comment.
Could this "1" constant be moved to the wasmtime_environ crate or similar?
There was a problem hiding this comment.
Either that or could there be a compile-time assertion that "1" is the pattern used here, with a comment saying that if the constant changes it needs to update the compiler too?
| pub(crate) fn component_task_state_mut(&mut self) -> &mut ComponentTaskState { | ||
| &mut self.component_data_mut().task_state | ||
| pub(crate) fn component_task_state_mut(&mut self) -> Result<&mut ComponentTaskState> { | ||
| // NB: Force the deferred lazy thread, if any, before handing out task |
There was a problem hiding this comment.
How come this is necessary? (same for the one below too)
There was a problem hiding this comment.
It's more defensive than strictly required as the code exists today. The problem is that we hand out ComponentTaskState which contains the concurrent state, which contains the unforced current thread, but by doing so the code that gets that state can't have access to the store at teh same time due to borrow rules so it can't get the actual/forced current thread, so we defensively update the unforced current thread eagerly before passing out the ComponentTaskState.
I think ideally we would completely remove the current thread from the concurrent state and only have it on store and refactor the rest of the code accordingly, but that seemed like a pretty big task to do in this PR.
There was a problem hiding this comment.
Code isn't expected to access unforced_current_thread though, right? I'd expect that if the current thread was needed it'd be done with StoreOpaque before getting ComponentTaskState, and by getting ComponentTaskState code would otherwise follow the convention of not even looking at unforced_current_state due to the scary name.
There was a problem hiding this comment.
Ideally, yes, but there is a lot of existing code that doesn't have access to a store and only has the ComponentTaskState. I felt it was better to be safe than sorry.
Some little fixes to stuff that was pointed out after bytecodealliance#13695 landed
Juggle some things around to remove an otherwise-needed promotion. Follow-up from bytecodealliance#13695
* Remove an accessor of `unforced_current_thread` Juggle some things around to remove an otherwise-needed promotion. Follow-up from bytecodealliance#13695 * Fix some test compiles * Defer requiring a scope to a bit later
2 participants
See each commit for details.