Should non-mMainThreadOnly Task be executed on main thread?
Categories
(Core :: XPCOM, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox117 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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?
// 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
:
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.
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
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;
Assignee | ||
Comment 1•1 years ago
|
||
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?
Comment 2•1 years ago
|
||
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).
Assignee | ||
Comment 3•1 years ago
|
||
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 | ||
Comment 4•1 year ago
|
||
Updated•1 year ago
|
Comment 6•1 year ago
|
||
Backed out for causing build bustages in idget/windows/nsWindow.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/828cec2f2ca04debd246e8652f86a6dba4df50b0
Comment 8•1 year ago
|
||
bugherder |
Assignee | ||
Updated•1 year ago
|
Description
•