Closed Bug 1839102 Opened 1 years ago Closed 1 year ago

Should non-mMainThreadOnly Task be executed on main thread?

Categories

(Core :: XPCOM, task)

task

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file)

TaskController's Task's comment says it can be executed on any thread including the main thread, but it looks like that's not true?

https://searchfox.org/mozilla-central/rev/05d1afda588e54be73e31ef4e022dde91f1ed97a/xpcom/threads/TaskController.h#113-119

// A Task is the the base class for any unit of work that may be scheduled.
// Subclasses may specify their priority and whether they should be bound to
// the Gecko Main thread. When not bound to the main thread tasks may be
// executed on any available thread (including the main thread), but they may
// also be executed in parallel to any other task they do not have a dependency
// relationship with. Tasks will be run in order of object creation.
class Task {

Task with mMainThreadOnly == false is added to mThreadableTasks is inserted into mThreadableTasks:

https://searchfox.org/mozilla-central/rev/05d1afda588e54be73e31ef4e022dde91f1ed97a/xpcom/threads/TaskController.cpp#416,456-459

void TaskController::AddTask(already_AddRefed<Task>&& aTask) {
...
  if (task->IsMainThreadOnly()) {
    insertion = mMainThreadTasks.insert(std::move(task));
  } else {
    insertion = mThreadableTasks.insert(std::move(task));

And mThreadableTasks's elements are handled only in TaskController::RunPoolThread, which seems to be run in separate thread.

https://searchfox.org/mozilla-central/rev/05d1afda588e54be73e31ef4e022dde91f1ed97a/xpcom/threads/TaskController.cpp#296-297,304-307,313-315

void TaskController::RunPoolThread() {
  IOInterposer::RegisterCurrentThread();
...
  nsAutoCString threadName;
  threadName.AppendLiteral("TaskController #");
  threadName.AppendInt(static_cast<int64_t>(mThreadPoolIndex));
  AUTO_PROFILER_REGISTER_THREAD(threadName.BeginReading());
...
    if (!mThreadableTasks.empty()) {
      for (auto iter = mThreadableTasks.begin(); iter != mThreadableTasks.end();
           ++iter) {

Also even the main-thread part TaskController::DoExecuteNextTaskOnlyMainThreadInternal skips Task with mMainThreadOnly == false

https://searchfox.org/mozilla-central/rev/05d1afda588e54be73e31ef4e022dde91f1ed97a/xpcom/threads/TaskController.cpp#770-771,802-803,815-817

bool TaskController::DoExecuteNextTaskOnlyMainThreadInternal(
    const MutexAutoLock& aProofOfLock) {
...
    for (auto iter = mMainThreadTasks.begin(); iter != mMainThreadTasks.end();
         iter++) {
...
      if (!task->IsMainThreadOnly() || task->mInProgress ||
          (task->mTaskManager && task->mTaskManager->mCurrentSuspended)) {
        continue;
Blocks: 1839108

In the context of bug 1839108, having a Task that's executed only off-main thread is suitable as direct replacement for the existing code where:

  • if the task won't take long, do it without creating separate Task (if a JS source is short, compile it immediately)
  • if the task may take long, create a task and do it in parallel with main thread (if a JS source is long, compile it off-main thread)

:Bas, can I have your input here?
How should a Task with mMainThreadOnly==false behave?
and if it should be executed on any thread including the main thread, is it feasible to have yet another kind of Task that's executed only off main thread?

Flags: needinfo?(bas)

In theory, that you are correct, however currently mMainThreadOnly==false means the task only ever gets executed off the main thread. This is because we found that some tasks in Gecko run off the main thread deadlock because they expect something to run on the main thread which they synchronously block on. Ideally you're right, we should have all three permutations, for now !mMainThreadOnly acts as mOffMainThreadOnly would (we should probably rename that and optionally add mMainThreadOnly again in the future).

Flags: needinfo?(bas)

thanks.

yes, I hit the dead-lock issue when I tried using TaskController in ScriptPreloader (bug 1836422) which indeed uses MonitorAutoLock between main thread vs off-main-thread task. and I used Runnable instead so far.

I'll look into making the bool parameter/field into enum class with descriptive name, and possibly TODO comment for the 3rd option.

Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
Flags: needinfo?(arai.unmht)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: