1704923 - Update HelperThreadTaskCallback interface

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

HelperThreadTaskCallback now takes a UniquePtr of RunnableTask
https://searchfox.org/mozilla-central/rev/3de2db87f3c9001ae478318d47a2ca3427574382/js/src/vm/Runtime.cpp#72
It was designed to submit the RunnableTask to XPCOM, and XPCOM will call its runTask() method.
However, for some tasks, they need to be traced.
https://searchfox.org/mozilla-central/rev/3de2db87f3c9001ae478318d47a2ca3427574382/js/src/vm/HelperThreads.cpp#2596

So it's easier to maintain those tasks in Spidermonkey, instead of XPCOM.
Therefore the parameter whose type is UniquePtr in HelperThreadTaskCallback doesn't make sense here.

We thought about changing it to raw pointer, RunnableTask* , but this also has a few problems:
Spidermonkey will cancel off-thread tasks when destroying the JS Runtime, which means we may need another abstraction to maintain the life cycle between the tasks from JS and XPCOM.

And as for IonCompileTask, it will always fetch the highest priority,
https://searchfox.org/mozilla-central/rev/3de2db87f3c9001ae478318d47a2ca3427574382/js/src/vm/HelperThreads.cpp#1822

And for ParseTask and CompressionTask, their tasks are processed in a LIFO order.
https://searchfox.org/mozilla-central/rev/d8194cbbeaec11962ed67f83aea9984bf38f7c63/js/src/vm/HelperThreads.cpp#1123
https://searchfox.org/mozilla-central/rev/d8194cbbeaec11962ed67f83aea9984bf38f7c63/js/src/vm/HelperThreads.cpp#1854

So for the tasks of these threadTypes, if we submit multiple tasks at once, then it could have different behavior as the original thread pool.

I am trying a simpler version, which is to change the API to HelperThreadTaskCallback(ThreadType), which takes the ThreadType as the parameter.

So Spidermonkey calls HelperThreadTaskCallback with the threadType of the JS off-thread task, and in bug 1703185 it will map the threadType to the EventQueuePriority so TaskController could know its priority accordingly. Then XPCOM calls a callback to notify Spidermonkey the task of the threadType is ready to be executed, Spidermonkey gets the next task from the specific threadType and execute it.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1704923 - Part 2: Add a threadType in dispatch().

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1704923 - Update HelperThreadTaskCallback to remove task parameter r=sfink!,KrisWright!

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1704923 - Part 3: Add terminate().

Comment 4

[Jon Coppeard (:jonco)]

The new plan is that the callback will not take any parameters and will instead run the highest priority JS task that is waiting. That makes it as close as possible to the current JS helper thread system (which also has the helper thread pick a task). Tighter integration with the XPCOM thread pool can happen later on.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1704923 - Move helper thread APIs to their own header file r?sfink

This also tidies up the implementation a bit, moving
SetHelperThreadTaskCallback to the JS namespace and removing the unused
RunnableTask class.

Depends on D112752

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1704923 - Pass the number of threads when setting up an external thread pool r=sfink!,bas!

The JS helper thread system needs to know how many threads are available, in
particular because parallel Wasm compilation needs at least two threads to
avoid deadlock. This adds a method to get the count from TaskController and
passes it through to the JS engine when setting up the thread pool.

Depends on D116219

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1704923 - Take account of thread count in concurrent task limits r?sfink

Currently the calculation of the maximum number of tasks for each kind assumes
that thread count is greater than or equal to CPU count. When using an external
thread pool that will no longer hold. To maintian the same behaviour we should
change those limits that use the currently more restrictive CPU limit to also
take the thread count limit into account.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59c3af3d6e7f
Update HelperThreadTaskCallback to remove task parameter r=sfink,KrisWright
https://hg.mozilla.org/integration/autoland/rev/fedb48e21c75
Move helper thread APIs to their own header file r=sfink
https://hg.mozilla.org/integration/autoland/rev/9eabc7fc9b81
Pass the number of threads when setting up an external thread pool r=sfink,bas
https://hg.mozilla.org/integration/autoland/rev/498f9f3927f9
Take account of thread count in concurrent task limits r=sfink

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing multiple failures in js::wasm::CompilerEnvironment::computeParameters

Backout link: https://hg.mozilla.org/integration/autoland/rev/dfbf93d7a24c2b9383e64d2a05a8cba2131c94ae

Push with failures

Failure log

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf5df28af277
Update HelperThreadTaskCallback to remove task parameter r=sfink,KrisWright
https://hg.mozilla.org/integration/autoland/rev/933c18440083
Move helper thread APIs to their own header file r=sfink
https://hg.mozilla.org/integration/autoland/rev/0ad1cec01d7b
Pass the number of threads when setting up an external thread pool r=sfink,bas
https://hg.mozilla.org/integration/autoland/rev/55601e9cd724
Take account of thread count in concurrent task limits r=sfink

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/cf5df28af277
https://hg.mozilla.org/mozilla-central/rev/933c18440083
https://hg.mozilla.org/mozilla-central/rev/0ad1cec01d7b
https://hg.mozilla.org/mozilla-central/rev/55601e9cd724