Add API and shell harness for cooperative multithreading

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted 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]
patch

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();

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

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 bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad2e60d7843
Add API and shell harness for cooperative multithreading, r=jandem.
https://hg.mozilla.org/mozilla-central/rev/fad2e60d7843
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1341317
Depends on: 1342642
Depends on: 1418189
You need to log in before you can comment on or make changes to this bug.