Skip to content
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

Task for compiling wasm modules could be ignored (but shouldn't be) #24771

Open
jdm opened this issue Nov 18, 2019 · 1 comment
Open

Task for compiling wasm modules could be ignored (but shouldn't be) #24771

jdm opened this issue Nov 18, 2019 · 1 comment
Labels
A-content/script Related to the script thread

Comments

@jdm
Copy link
Member

jdm commented Nov 18, 2019

See discussion starting at #24757 (comment). The TaskQueue infrastructure used by the various task sources can cause the task to be ignored for an unbounded period of time; we should find a better mechanism that provides the guarantees that the JS engine requires.

@jdm jdm added the A-content/script Related to the script thread label Nov 18, 2019
@jdm jdm changed the title Task for compiling wasm modules can be ignored Task for compiling wasm modules could be ignored (but shouldn't be) Nov 18, 2019
@gterzian
Copy link
Member

gterzian commented Nov 19, 2019

Actually this made me realize a flaw in the current TaskQueue: #24780

And an initial solution(once the above issue has been addressed) might indeed be to put the message containing the wasm module dispatchable to be a CommonScriptMsg that is not a task.

That would still give us only partial guarantee the message is eventually handled and run called on the wasm module, because both window and worker event-loops have an "early return" logic when they are supposed to exit. Workers use the is_closing check, while the ScriptThread simply returns early once it receives the ExitScriptThread message from the constellation.

So in both cases, the wasm module message could end-up somewhere at the back of the queue, while in the middle there is an "exit" message of some sort that is handled first, in which case we can't be sure the channel will be drained.

To solve this, we could either move the wasm module message to a dedicated 0 bounded channel, and block until the message is either handled, or the channel dropped on exit. I'm not sure it's a good idea to block the SpiderMonkey wasm compilation helper thread though.

So maybe we want to have not a channel but some dedicated shared state, like:

enum WasmCompilationChannel {
    Open(VecDeque<Runnable>),
    Closed
}

and share that with the callback in a thread-safe way, and then commit all event-loops to always drain the queue when the state is set to Closed, which would happen once the event-loop determines that it will shutdown.

This would actually have to be combined with sending a message on a script-chan, to ensure the event-loop wakes-up when a Runnable is added to a queue(and that "wake-up" message doesn't have to be handled, so long as the event-loop commits to draining the queue if it receives another message first telling it to exit).

it would also require calling runnable.run(RustRuntime::get(), Dispatchable_MaybeShuttingDown) on all runnables in the queue, at some point inside the event-loop, where Dispatchable_MaybeShuttingDown would be set to the proper value based on whether the event-loop is shutting down or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/script Related to the script thread
Projects
None yet
Development

No branches or pull requests

2 participants