Closed Bug 1337968 Opened 4 years ago Closed 4 years ago

Add API and shell harness for cooperative multithreading


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox54 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)




(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
After the previous patches in bug 1323066, everything is finally in place to actually have multiple cooperating contexts/threads in a runtime.  The attached patch adds two new APIs, JS_NewCooperativeContext and JS_YieldCooperativeContext, and expands the worker thread harness in the JS shell to allow for creation and use of these contexts via the evalInCooperativeThread() and cooperativeYield() functions (the browser will yield via the interrupt callback but that isn't necessary for the test harness I think).  Each thread created this way is given its own zone group and doesn't (or, more likely, shouldn't) yet have a way to communicate with other threads, though they can GC each others' contents.
Attachment #8835162 - Flags: review?(jdemooij)
Comment on attachment 8835162 [details] [diff] [review]

Review of attachment 8835162 [details] [diff] [review]:

Looks reasonable but a suggestion below.

(In reply to Brian Hackett (:bhackett) from comment #0)
> (the browser will
> yield via the interrupt callback but that isn't necessary for the test
> harness I think).

There's a shell function setInterruptCallback(function) to set a function that will get called by the shell's interrupt callback. Can't hurt to add a test that uses setInteruptCallback(cooperativeYield)

::: js/src/jsapi.h
@@ +999,5 @@
> +// Yield control of a runtime from one thread holding a cooperative context to
> +// another. The old context must be the current active context in the runtime,
> +// and after this call it may no longer use the JS API until control is yielded
> +// back to it, while the new context is free to use the API. The new context is
> +// optional.

The comment says newContext is optional, so I thought oldContext was always non-null but that's not true. It's probably worth documenting that better. Also, can we assert at least one of them is non-null?

Thinking about it more, it's a bit surprising to use YieldCooperativeContext when we resume execution. It might be cleaner to have two APIs, each with a non-null cx argument:

- JS_YieldCooperativeContext(activeContext)
- JS_ResumeCooperativeContext(newContext)

::: js/src/shell/js.cpp
@@ +3620,5 @@
>      }
> +    if (cooperative) {
> +        if (!cooperationState)
> +            cooperationState = new CooperationState();

Attachment #8835162 - Flags: review?(jdemooij)
Attached patch patchSplinter Review
Attachment #8835162 - Attachment is obsolete: true
Attachment #8836198 - Flags: review?(jdemooij)
Comment on attachment 8836198 [details] [diff] [review]

Review of attachment 8836198 [details] [diff] [review]:

Sorry for the delay and thanks for making these changes.
Attachment #8836198 - Flags: review?(jdemooij) → review+
Pushed by
Add API and shell harness for cooperative multithreading, r=jandem.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.