-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Minimal Worker support #1476
Minimal Worker support #1476
Conversation
This adds the ability to spawn additional Isolates from Rust and send and receive messages from them. This is preliminary work to support running the typescript compiler in a separate isolate and thus support native ES modules (denoland#975).
Box::new(futures::future::err(err)) | ||
} | ||
|
||
// https://github.com/denoland/deno/blob/golang/os.go#L100-L154 | ||
fn op_code_fetch( | ||
state: &IsolateState, | ||
state: &Arc<IsolateState>, |
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 wonder if we should refactor so that IsolateState itself has Send, instead of wrapping in an Arc everywhere. After all the Mutex is already internalized...
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.
Sure. These types of refactors are generally easy to do due to the type system. I'd leave this as is for now because of laziness...
} | ||
} | ||
|
||
fn op_worker_get_message( |
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.
Seeing this does make me feel a bit stronger in favor of a unified messaging substrate instead of having distinct ones.
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.
We do have a unified messaging substrate; getMessage
and postMessage
are built on top of the lower-level op dispatch system.
I'd rephrase your suggestion: "postMessage
should be the means that we dispatch ops."
I'm not so convinced of that. I think we have very particular concerns in the IPC system that the WebWorkers API doesn't address like synchronous responses, zero-copy buffers.
|
||
/// Rust interface for WebWorkers. | ||
pub struct Worker { | ||
isolate: Isolate, |
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.
Is Isolate itself just a wrapper around a pointer, or a class with lots of data members?
Asking because if the latter is the case, this inflates every entry in the resource table to something potentially huge.
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.
Yes, it's a class with data members - it's not just a pointer. Your comment is valid.
I agree that the resource table should have Box
elements instead of directly containing structs - but I'd rather do that refactor in a different PR.
pub fn add_worker(wc: WorkerChannels) -> Resource { | ||
let rid = new_rid(); | ||
let mut tg = RESOURCE_TABLE.lock().unwrap(); | ||
let r = tg.insert(rid, Repr::Worker(wc)); |
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.
Same question as we had once with HTTP -- where does the worker get removed from the resource 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.
They weren't being correctly removed. I've fixed this now. When the thread exits it calls resource.close()
which removes it from the table. I've added a test to ensure this.
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.
Nice work! Surely this wasn't easy to pull off...
See comments (not much). Most important: the worker resource get released?
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.
🎉 Hooray! Workers!
This adds the ability to spawn additional Isolates from Rust and send
and receive messages from them. This is preliminary work to support
running the typescript compiler in a separate isolate and thus support
native ES modules (#975).
This is a prerequisite commit to #1460