Skip to content

[worklets] don't run scripts from "in parallel" #370

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

Closed
annevk opened this issue Apr 5, 2017 · 4 comments · Fixed by #375
Closed

[worklets] don't run scripts from "in parallel" #370

annevk opened this issue Apr 5, 2017 · 4 comments · Fixed by #375
Assignees

Comments

@annevk
Copy link
Member

annevk commented Apr 5, 2017

import() tries to do this, that doesn't work and leads to all kinds of undefined behavior. When you run script the world needs to be in some kind of stable state.

@bfgeek
Copy link
Contributor

bfgeek commented Apr 5, 2017

Right but each WorkletGlobalScope could be in a different thread. Workers do this by saying:
"Create a separate parallel execution environment (i.e. a separate thread or process or equivalent construct), and run the rest of these steps in that context."

We could instead say:
"Each WorkerGlobalScope as a execution environment which may be parallel (i.e. it may be on a separate thread or process, it also may be on the same thread or process). Which thread this lives on is decided by the user-agent."

And inside the for-loop inside import:
"Run the following steps in context of the WorkletGlobalScope's execution environment".

/cc @domenic

@annevk
Copy link
Member Author

annevk commented Apr 5, 2017

Yeah, that sounds reasonable, but import() can be invoked after one is created and is already running script, no? So you first need to synchronize across those possible threads somehow (usually done via message passing) before you can start running things.

@bfgeek
Copy link
Contributor

bfgeek commented Apr 5, 2017

import() in this case is only ever called from the main thread, so subsequent calls would run in the right order? This doesn't need to be explicit right?

(also did you want to review this change at all?)

@annevk
Copy link
Member Author

annevk commented Apr 5, 2017

Happy to review PRs (I found out you have a few outstanding PRs without any kind of status, might want to check those).

You do need to be explicit about ordering, since with "in parallel" the second call could happen before the first. No ordering guarantees. Also, the second call could finish during the execution of the first script, at which point you obviously can't run script, etc.

(@domenic this is another reason we need "in parallel" to become threads with message passing.)

bfgeek added a commit that referenced this issue Apr 6, 2017
Instead of relying on "in parallel" this change makes all the thread
hopping explicit via. message passing and queue tasks on the appropriate
event loop.

 - Fixes #372, changes arguments to fetch a module script graph.

 - Fixes #370, running scripts in parallel. Now the script explicitly are
   run within a task queued on the worklet global scope's event loop.

 - Fixes #318, actually runs the event loop.

 - Fixes #230, treats "fetch a module worker script graph" as
   asynchronous.

 - Probably fixes #225 ? I think I'm grabbing the correct state at each
   thread hop.
@tabatkins tabatkins added the ready label Apr 6, 2017
bfgeek added a commit that referenced this issue Apr 11, 2017
* [worklets] Multiple fixes to the {{Worklet/import()}} algorithm.

Instead of relying on "in parallel" this change makes all the thread
hopping explicit via. message passing and queue tasks on the appropriate
event loop.

 - Fixes #372, changes arguments to fetch a module script graph.

 - Fixes #370, running scripts in parallel. Now the script explicitly are
   run within a task queued on the worklet global scope's event loop.

 - Fixes #318, actually runs the event loop.

 - Fixes #230, treats "fetch a module worker script graph" as
   asynchronous.

 - Probably fixes #225 ? I think I'm grabbing the correct state at each
   thread hop.
@tabatkins tabatkins removed the ready label Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants