-
Notifications
You must be signed in to change notification settings - Fork 1.6k
consistently create thread and task when entering component instance #12379
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
Conversation
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
DetailsTo modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
While rebasing bytecodealliance/wasmtime#12379 onto Wasmtime's `main` branch, I found I needed to tweak the expected resource handle values and trap messages due to subtle changes to when Wasmtime allocates a thread handle. Once WebAssembly#600 lands and is implemented in Wasmtime, we should be able to clean all this up once and for all; for now we just muddle along.
|
I've rebased this onto main and addressed the feedback so far locally. Once WebAssembly/component-model#601 is merged, I'll push my updates. |
While rebasing bytecodealliance/wasmtime#12379 onto Wasmtime's `main` branch, I found I needed to tweak the expected resource handle values and trap messages due to subtle changes to when Wasmtime allocates a thread handle. Once #600 lands and is implemented in Wasmtime, we should be able to clean all this up once and for all; for now we just muddle along.
ea0b3fb to
200c24a
Compare
alexcrichton
left a comment
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.
Two other possible spots worth scrutinizing:
cabi_reallocinvocations - probably don't need special treatment? Unsure.ResourceAny::resource_drop- I think this'll need task-related treatment?
Although given the litany of entrypoints into wasm I'm finding it really hard to keep them all in sync, so it's probably also worth refactoring in theory to only have one entrypoint somewhere
As with post-return functions, we don't allow
Yup, I'll address that and add a test.
I'll open an issue for that. |
|
Right yeah |
Previously, we weren't creating a new thread or task in all cases when entering a component instance, even when component model async features were enabled. In particular, entering an instance via a sync-to-sync, guest-to-guest adapter, via `Linker::instantiate[_async]`, or via `[Typed]Func::call` all skipped creating a thread or task, creating panics and/or instance mismatches in certain cases. This commit addresses all those cases and also adds assertions to all CM async intrinsics to verify that the caller instance matches the most-recently-pushed task. Note that we still skip pushing and popping threads and tasks if no CM async features are enabled in the `Config`. In order to populate the `GuestTask::instance` field for tasks created as part of `Linker::instantiate[_async]` calls, I had to add a `RuntimeComponentInstanceIndex` field to `GlobalInitializer::InstantiateModule` and friends so it would be available when needed. While testing this, I uncovered and fixed a couple of related issues: - We weren't checking the `may_leave` flag when guest-to-guest calling a resource destructor - We weren't checking whether a subtask was ready to delete (e.g. that no threads were still running) before attempting to delete it while deleting its supertask
6be1581 to
f264784
Compare
…tions ...and assert that the indexes match as expected. Getting the post-return test to pass required moving the call to `StoreOpaque::exit_sync_call` to after the post-return function is called.
This commit is an extension/refactor of bytecodealliance#12377 and bytecodealliance#12379. Notably this decouples the runtime behavior of Wasmtime from enabled/disabled WebAssembly proposals. This enables the `wasmtime serve` subcommand, for example, to continue to disallow component-model-async by default but continue to use `*_concurrent` under the hood. Specifically a new `Config::concurrency_support` knob is added. This is plumbed directly through to `Tunables` and takes over the preexisting `component_model_concurrency` field. This field configures whether tasks/etc are enabled at runtime for component-y things. The default value of this configuration option is the same as `cfg!(feature = "component-model-async")`, and this field is required if component-model-async wasm proposals are enabled. It's intended that eventually this'll affect on-by-default wasm features in Wasmtime depending if the support is compiled in. This results in a subtle shift in behavior where component-model-async concurrency is used by default now because the feature is turned on by default, even though the wasm features are off-by-default. This required adjusting a few indices expected in runtime tests due to tasks/threads being allocated in index spaces. Finally, this additionally denies access at runtime to `Linker::*_concurrent` when concurrent support is disabled as otherwise the various runtime data structures won't be initialized and panics will happen. Closes bytecodealliance#12393
* Document panics from using CM async machinery when CM async is not enabled * Refactor how concurrency support is enabled in a `Store` This commit is an extension/refactor of #12377 and #12379. Notably this decouples the runtime behavior of Wasmtime from enabled/disabled WebAssembly proposals. This enables the `wasmtime serve` subcommand, for example, to continue to disallow component-model-async by default but continue to use `*_concurrent` under the hood. Specifically a new `Config::concurrency_support` knob is added. This is plumbed directly through to `Tunables` and takes over the preexisting `component_model_concurrency` field. This field configures whether tasks/etc are enabled at runtime for component-y things. The default value of this configuration option is the same as `cfg!(feature = "component-model-async")`, and this field is required if component-model-async wasm proposals are enabled. It's intended that eventually this'll affect on-by-default wasm features in Wasmtime depending if the support is compiled in. This results in a subtle shift in behavior where component-model-async concurrency is used by default now because the feature is turned on by default, even though the wasm features are off-by-default. This required adjusting a few indices expected in runtime tests due to tasks/threads being allocated in index spaces. Finally, this additionally denies access at runtime to `Linker::*_concurrent` when concurrent support is disabled as otherwise the various runtime data structures won't be initialized and panics will happen. Closes #12393 * Add a `-Wconcurrency-support` CLI flag Used to update disas tests to show that, when disabled, old codegen quality is preserved * Ungate `Config` flag * Review comments --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
Previously, we weren't creating a new thread or task in all cases when entering a component instance, even when component model async features were enabled. In particular, entering an instance via a sync-to-sync, guest-to-guest adapter, via
Linker::instantiate[_async], or via[Typed]Func::callall skipped creating a thread or task, leading to panics and/or instance mismatches in certain cases.This commit addresses all those cases and also adds assertions to all CM async intrinsics to verify that the caller instance matches the most-recently-pushed task. Note that we still skip pushing and popping threads and tasks if no CM async features are enabled in the
Config.In order to populate the
GuestTask::instancefield for tasks created as part ofLinker::instantiate[_async]calls, I had to add aRuntimeComponentInstanceIndexfield toGlobalInitializer::InstantiateModuleand friends so it would be available when needed.While testing this, I uncovered and fixed a couple of related issues:
may_leaveflag when guest-to-guest calling a resource destructor