Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Take into account requirements for "isolate" or single-threaded runtimes #5284

Draft
wants to merge 19 commits into
base: wasmer-5.1.0
Choose a base branch
from

Conversation

xdoardo
Copy link
Contributor

@xdoardo xdoardo commented Dec 5, 2024

V8 needs Wasm entities to be created in the same thread they run in. This PR moves the creation of said entities down to the task_wasm function in WASIX.

Caveats

This depends on a build of V8 with shared memory, which is not available yet - that is, running the precise code in this PR with wordpress won't work (again, because the wasm_c_api implementation of V8 does not support shared memories)

#[cfg(feature = "js")]
Self::Js(_) => crate::RuntimeKind::Js,
#[cfg(feature = "jsc")]
Self::Jsc(_) => crate::RuntimeKind::Jsc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is public, it needs to be #[non_exhaustive]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -19,6 +19,9 @@ pub(crate) struct StoreInner {
pub(crate) on_called: Option<OnCalledHandler>,
}

unsafe impl Send for StoreInner {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should very much not have this on the whole StoreInner.

If we need it for one particular variant, it should be on that variant. (v8)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it in c463031. BTW, v8 is technically the only one that should not be Send.

let store_ref = store.as_store_ref();
let v8_store = store_ref.inner.store.as_v8();

if v8_store.thread_id != std::thread::current().id() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used a lot... how about making it a method?

task.spawn_type,
task.update_layout,
)?;
//let module = task.module;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -240,6 +240,11 @@ impl crate::Engine {
_ => panic!("Not a `jsc` engine!"),
}
}

/// Return true if [`self`] is an engine from the `jsc` runtime.
pub fn is_jsc(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the js engine have a is_jsc method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in jsc/entities/engine.rs. Am I missing something?

@xdoardo xdoardo linked an issue Dec 6, 2024 that may be closed by this pull request
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime: take into account single-threaded requirements
2 participants