1706357 - Refactor the maybeGetXXXTask in GlobalHelperThreadState

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, task, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

The GlobalThreadState's HelperThreads uses a list of selectors to run tasks.
https://searchfox.org/mozilla-central/rev/b6f52976b562008c9d9ceeda22907e1eda506c8e/js/src/vm/HelperThreads.cpp#2659

For example, maybeGetGCParallelTask
https://searchfox.org/mozilla-central/rev/b6f52976b562008c9d9ceeda22907e1eda506c8e/js/src/vm/HelperThreads.cpp#1916

The pattern is:
if "task queue is empty" or "checkTaskThreadLimit failed"
return
pop up the next from the task queue.

We could refactor these maybeGetXXXTask into several helper functions, so when dispatching tasks to the external thread pool, we could reuse these helper functions.

For the external thread pool, TaskController, should handle the cases "task queue is empty" and "checkTaskThreadLimit failed" differently.

In the case of task queue is empty, it means either the task is cancelled or being run on main thread, so we don't have to run it again, we could return true.

For the case of checkTaskThreadLimit failed, we still need to run the task, except we cannot run it because of the thread parallelism limitation, we need to return false.

See the comments from mozilla::Task::Run
https://searchfox.org/mozilla-central/rev/b6f52976b562008c9d9ceeda22907e1eda506c8e/xpcom/threads/TaskController.h#192

The refactoring here will make bug 1704923 more easily.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1706357 - Part 1: Add isTaskQueueEmpty.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1706357 - Part 2: Add calcMaxWasmCompilationThreads.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1706357 - Part 3: Add another version of checkTaskThreadLimit that takes ThreadType.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1706357 - Part 4: Add getTask().

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1706357 - Part 5: Add priorities[] and maybeGetTask.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1706357 - Part 6 : Remove dead code: maybeGetXXXTask.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #0)

For the case of checkTaskThreadLimit failed, we still need to run the task, except we cannot run it because of the thread parallelism limitation, we need to return false.
See the comments from mozilla::Task::Run

This says:

  // When this returns false, the task is considered incomplete and will be
  // rescheduled at the current 'mPriority' level.

Do you know what the effect this has? Does it just immediately run the task if there are no others pending? It would be bad if we dispatched a task and had it effectively spinning until some other task finished.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #7)

Do you know what the effect this has? Does it just immediately run the task if there are no others pending? It would be bad if we dispatched a task and had it effectively spinning until some other task finished.

Yes, it will be spinning, but the spinning time shouldn't be too long, and this is just the first step to integrate with TaskController.
I'll reopen bug 1704534 in XPCOM to address this. (I just closed it today)

But we still need the patches in this bug to factor out the checkTaskThreadLimit part, to tell the TaskController to run this task later, if you agree.

The main idea is to refactor GlobalHelperThreadState (this bug) and update the interface (bug 1704923) so we can do more experiments and more elaborations with TaskController, and we still dispatch to our existing HelperThreads by default (bug 1703240) so it shouldn't break any existing behavior.
So with these patches from those bugs, for a off-thread Task, whether it's run from our internal HelperThread, or from an external XPCOM thread, they both call (almost) the same functions: isTaskQueueEmpty(), checkTaskThreadLimit, getTask, and runHelperThreadLocked, this should help us maintaining easier.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #8)

Yes, it will be spinning, but the spinning time shouldn't be too long, and this is just the first step to integrate with TaskController.

Can we avoid dispatching tasks to the controller until they are ready to run? Dispatching tasks only to have them spinning is wasteful of CPU resources and something we want to avoid. I understand this is only the first step, but this is an important aspect to figure out.

The main idea is to refactor GlobalHelperThreadState (this bug) and update the interface (bug 1704923) so we can do more experiments and more elaborations with TaskController, and we still dispatch to our existing HelperThreads by default (bug 1703240) so it shouldn't break any existing behavior.
So with these patches from those bugs, for a off-thread Task, whether it's run from our internal HelperThread, or from an external XPCOM thread, they both call (almost) the same functions: isTaskQueueEmpty(), checkTaskThreadLimit, getTask, and runHelperThreadLocked, this should help us maintaining easier.

I agree with all of this.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #9)

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #8)

Yes, it will be spinning, but the spinning time shouldn't be too long, and this is just the first step to integrate with TaskController.

Can we avoid dispatching tasks to the controller until they are ready to run? Dispatching tasks only to have them spinning is wasteful of CPU resources and something we want to avoid. I understand this is only the first step, but this is an important aspect to figure out.
I agree with all of this.

Cool, then the remaining question is the should we call checkTaskThreadLimit before dispatching.

Yes, we could try to avoid that before dispatching them, but our existing GlobalHelperThreadState does this on the helper thread already,
that means either
A: We need to change our GlobalHelperThreadState scheduling policy to make it have the same behavior with TaskController,
or
B: We need to maintain two scheduling policies in our GlobalHelperThreadState, one for internal, and the other for external.

It looks like you mean B? I'll file another bug for addressing this.

From the point of view of Spidermonkey, it should just submit those tasks. Whereas the policy, the limitation,..etc should be handled by the scheduler, not by Spidermonkey itself. We tried to do the scheduling before dispatching last year, but at that time it was still using nsThreadPool, which doesn't have handle priority of tasks, and other several problems, so it was easier to do that in SM. But now we're using TaskController, which should be used by the whole Gecko, IMO it would be easier and more beneficial for the SM and Gecko to implement the parallelism limitation in TaskController.
Anyway, I'll purpose patches for both sides (SM and TaskControll), and then decide which one we really want.

But whether we're going to fix the parallelism limitation in SM or TaskControll, we still need the patches from this bug and bug 1704923.
I could update Bug 1704923 to make it not call HelperThreadTaskCallback at all (but again, we still need to change the parameter to js::ThreadType) even someone could export the env var: MOZ_JS_DISPATCH_TO_EXTERNAL_THREAD_POOL by itself.

Comment 11

[Jon Coppeard (:jonco)]

We're not going to take this approach at this time.