-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Cooperative Multithreading #11751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Cooperative Multithreading #11751
Conversation
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:config"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments from review, mostly pretty minor. I'm ccing @dicej on this as well as he's the main author of concurrent.rs
, the meat of the changes here, so he'll be taking a look at that
@@ -1,5 +1,6 @@ | |||
;;! component_model_async = true | |||
;;! component_model_async_stackful = true | |||
;;! component_model_threading = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #11818 I'm hoping to split tests apart based on features rather than having everything in one large test, so mind adding the updated version of this test to a new test instead of updating the one here?
@@ -1,5 +1,6 @@ | |||
;;! component_model_async = true | |||
;;! component_model_async_builtins = true | |||
;;! component_model_threading = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this, and a few updates above, may not be necessary any more? (maybe once were but may no longer be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be an accidental update? (assuming based on the removed test)
(import "async" "thread-yield" (func $thread-yield)) | ||
(func (export "run") | ||
call $yield | ||
call $thread-yield |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be an unrelated victim of "mass rename yield to thread-yield" (it's core modules not components)
#[tokio::test] | ||
async fn missing_task_return_call_stackless_explicit_thread() -> Result<()> { | ||
task_return_trap( | ||
r#"(component | ||
(core module $libc | ||
(table (export "__indirect_function_table") 1 funcref)) | ||
(core module $m | ||
(import "" "task.return" (func $task-return)) | ||
(import "" "thread.new_indirect" (func $thread-new-indirect (param i32 i32) (result i32))) | ||
(import "" "thread.resume-later" (func $thread-resume-later (param i32))) | ||
(import "libc" "__indirect_function_table" (table $indirect-function-table 1 funcref)) | ||
(func $thread-start (param i32) (; empty ;)) | ||
(elem (table $indirect-function-table) (i32.const 0) func $thread-start) | ||
(func (export "foo") (result i32) | ||
(call $thread-resume-later | ||
(call $thread-new-indirect (i32.const 0) (i32.const 0))) | ||
i32.const 0 | ||
) | ||
(func (export "callback") (param i32 i32 i32) (result i32) unreachable) | ||
) | ||
(core instance $libc (instantiate $libc)) | ||
(core type $start-func-ty (func (param i32))) | ||
(alias core export $libc "__indirect_function_table" (core table $indirect-function-table)) | ||
(core func $thread-new-indirect | ||
(canon thread.new_indirect $start-func-ty (table $indirect-function-table))) | ||
(core func $thread-resume-later (canon thread.resume-later)) | ||
(core func $task-return (canon task.return)) | ||
(core instance $i (instantiate $m | ||
(with "" (instance | ||
(export "thread.new_indirect" (func $thread-new-indirect)) | ||
(export "thread.resume-later" (func $thread-resume-later)) | ||
(export "task.return" (func $task-return)) | ||
)) | ||
(with "libc" (instance $libc)) | ||
)) | ||
(func (export "foo") (canon lift (core func $i "foo") async (callback (func $i "callback")))) | ||
)"#, | ||
"wasm trap: async-lifted export failed to produce a result", | ||
) | ||
.await | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that this is preexisting (my comment here applies to other tests in this file too) and so you can defer this to later if you desire, but these tests are perfect for adding to tests/misc_testsuite/**/*.wast
vs in-Rust tests here. Should be able to (assert_trap (invoke ...) "the message")
and that makes the tests much easier to run and avoids compiling more Rust code for tests
func.post_return_async(store.as_context_mut()).await?; | ||
} | ||
|
||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comment below, but this would also be a good test to add to the *.wast
-based testing. It might be a little more verbose there but I think it'd be worth it (ideally all tests are *.wast
).
Feel free to defer this to a future PR though
r#" | ||
(module $m2 | ||
(import "" "yield" (func $yield)) | ||
(import "" "thread-yield" (func $thread-yield)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be another victim of a mass rename (unrelated to components here)
r#" | ||
(component | ||
(import "yield" (func $yield (result (list u8)))) | ||
(import "thread-yield" (func $thread-yield (result (list u8)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically I think a victim of the mass renabme and while this is related to components it's host-level yielding as opposed to component-level yielding.
#[cfg(feature = "component-model-async")] | ||
fn thread_switch_to( | ||
store: &mut dyn VMStore, | ||
instance: Instance, | ||
caller: u32, | ||
cancellable: u8, | ||
thread_idx: u32, | ||
) -> Result<bool> { | ||
store.component_async_store().suspension_intrinsic( | ||
instance, | ||
RuntimeComponentInstanceIndex::from_u32(caller), | ||
cancellable != 0, | ||
false, | ||
Some(TableId::new(thread_idx)), | ||
) | ||
} | ||
|
||
#[cfg(feature = "component-model-async")] | ||
fn thread_suspend( | ||
store: &mut dyn VMStore, | ||
instance: Instance, | ||
caller: u32, | ||
cancellable: u8, | ||
) -> Result<bool> { | ||
store.component_async_store().suspension_intrinsic( | ||
instance, | ||
RuntimeComponentInstanceIndex::from_u32(caller), | ||
cancellable != 0, | ||
false, | ||
None, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like it should be possible to reduce the number of functions here to correspond to just the suspension_intrinsic
function. There's no need for the libcalls here to be 1:1 with component model libcalls as the compiler could translate, for example, the thread_suspend
call here into a suspension_intrinsic
libcall which would bake in false
as a runtime argument in the trampoline and this function itself would take the boolean an an argument.
Mostly a stylistic preference one way or another and I don't mind either way myself. Just wanted to note the possibility in case you didn't like having so many intrinsics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great; thanks for doing this!
I've only looked closely at the concurrent.rs
changes; Alex said the rest looked fine. It all looks reasonable to me; just a few inline comments here and there. I appreciate the tweaks you made to the existing code to make it more readable.
|
||
/// Helper function for the `waitable-set.wait`, `waitable-set.poll`, and | ||
/// `yield` intrinsics. | ||
fn pending_cancellation(&self, store: &mut StoreOpaque) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self
doesn't appear to be used here; perhaps this function should be moved to impl ConcurrentState
?
|
||
/// Implements the `thread.yield` intrinsic. | ||
pub(crate) fn thread_yield( | ||
unsafe fn read_funcref_from_table( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of unsafe
code here that I don't have enough context for. @alexcrichton would you mind talking a look?
/// populated as needed. | ||
// TODO: this can and should be a `PrimaryMap` | ||
instance_states: HashMap<RuntimeComponentInstanceIndex, InstanceState>, | ||
/// The "high priority" work queue for this instance's event loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The "high priority" work queue for this instance's event loop. | |
/// The "high priority" work queue for this store's event loop. |
Not related to your changes, but I should have updated these docs when I moved ConcurrentState
to StoreOpaque
.
/// The "high priority" work queue for this instance's event loop. | ||
high_priority: Vec<WorkItem>, | ||
/// The "high priority" work queue for this instance's event loop. | ||
/// The "low priority" work queue for this instance's event loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The "low priority" work queue for this instance's event loop. | |
/// The "low priority" work queue for this store's event loop. |
|
||
/// Implements the `thread.index` intrinsic. | ||
pub(crate) fn thread_index(&self) -> Result<u32> { | ||
Ok(self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaks information "non-determinstically" about how many table indexes have been allocated across all (sub)components by returning the rep directly. We should translate the rep to a (sub)component-specific handle by e.g. using a RuntimeComponentInstanceIndex
-specific table like we do for subtask
, stream
, future
, resource
, etc. handles. Basically any way we can generate a unique integer as a deterministic function of the (sub)component that created the thread (without leaking information about other (sub)components) should be fine.
Happy to discuss further if this isn't clear.
} | ||
|
||
impl GuestTask { | ||
fn have_lowered_parameters(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional nit: task.have_lowered_parameters()
feels a bit ungrammatical. Perhaps change this to has_*
instead?
Implements WebAssembly/component-model#557
Submitting as a draft PR for now to share progress.
GuestTask
GuestCall
functionality for new threadscomponent_model_async_builtins
andcomponent_model_async_stackful
features