Closed Bug 1888429 Opened 3 months ago Closed 28 days ago

TaskController tasks don't map directly to JS helper thread tasks

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
128 Branch
Tracking Status
firefox128 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

The JS helper thread system has a queue of tasks which it executes with the help of a system thread pool, which is TaskController in browser builds. The tasks dispatched to the thread pool call back into the JS engine to execute whatever the highest priority helper thread task is in the queue. Since this happens when the thread pool task runs, these tasks aren't associated with any particular JS task until they execute.

This has two problems:

  • from debugging / performance investigation POV you can't tell what a TaskController task is going to do before it starts executing
  • you can't pass thread priority / request a performance profile for the task

We should investigate picking the JS task when dispatching the TaskController task instead.

Severity: -- → N/A
Priority: -- → P3

This was added a while ago to avoid waiting for a helper thread to start if the
helper thread system is busy. However it is not compatible with the changes in
this bug.

This does make a difference to parallel marking on the linux system I tested
on, so the patch changes that to explicitly use the main thread as one of the
marking threads (this always happened previously since we don't release the
lock before waiting on it). I belive this is because the core the main thread
is running on is likely to already have a bunch of the relevant data in its
caches.

This changes the helper thread system to pick the task to dispatch up front and
pass it though to TaskController via the callback.

One issue that came up was that the memory containing IonCompileTasks is
protected until the task starts running, but we may now trace these before that
happens.

One thing I noticed while working on this was how much of the internals of
GlobalHelperThreadState are public. Code that access these internals can be
refactored into methods which also simplifies this code.

This changes the shell's internal thread pool to dispatch tasks to helper threads in
a fixed order rather than by threads picking up each task depending on which is
the first to get scheduled. In other words, if there is only ever at most one
task in the system then all tasks will be run on the first pool thread; if
there are at most two then the only the first two threads will be used.

This results in a noticable improvement on the splay benchmark on my linux
system, probably due to caching effects. This patch only affects the shell but
it indicates that it may be preferable for TaskController to work in a
similar way.

Attachment #9395249 - Attachment description: Bug 1888429 - Part 1: Remove optimsation where we run helper thread tasks on the main thread if they haven't started by the time we wait for them r?jandem → Bug 1888429 - Part 1: Remove optimization where we run helper thread tasks on the main thread if they haven't started by the time we wait for them r?jandem
Attachment #9395252 - Attachment description: Bug 1888429 - Part 4: Make the intenral thread pool dispatch tasks to free threads in a fixed order r?jandem → Bug 1888429 - Part 4: Make the internal thread pool dispatch tasks to free threads in a fixed order r?jandem

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

This results in a noticable improvement on the splay benchmark on my linux
system, probably due to caching effects.

I'm not sure this will reproduce in CI or on other Linux systems, but what I'm currently seeing is 3.5% improvement in Splay and 6.3% improvement in SplayLatency over 20 runs.

Running this under perf shows a 48% reduction in CPU migrations with these patches, although it also shows a 17% increase in stalled-cycles-backend and a 9% increase in cycles.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b64446651983
Part 1: Remove optimization where we run helper thread tasks on the main thread if they haven't started by the time we wait for them r=jandem
https://hg.mozilla.org/integration/autoland/rev/c82b988fab50
Part 2: Pass task to run to helper thread dispatch callback r=jandem,mccr8
https://hg.mozilla.org/integration/autoland/rev/8b30cb54d5d9
Part 3: Refactor helper thread code to make more GlobalHelperThreadState members private r=jandem
https://hg.mozilla.org/integration/autoland/rev/4c5efaca4057
Part 4: Make the internal thread pool dispatch tasks to free threads in a fixed order r=jandem

These patches caused a deadlock waiting on a mutex. Bug 1853400 could help here.

Flags: needinfo?(jcoppeard)
No longer regressions: 1893775

(In reply to Jon Coppeard (:jonco) from comment #8)
Bug 1853400 did not help here.

Depends on: 1894882
Attachment #9395250 - Attachment description: Bug 1888429 - Part 2: Pass task to run to helper thread dispatch callback r?jandem → Bug 1888429 - Part 1: Pass task to run to helper thread dispatch callback r?jandem

This removes the check from checkTaskThreadLimit that always allows tasks to
start if |maxThreads == threadCount|. Previously there was a further check on
the number of dispatched tasks that limited this to the number of threads but
that was removed in the previous patch. I don't know why this check was ever
there.

Without this change we will dispatch helper thread tasks for all Ion
compilations which can result in a very large number of tasks being dispatched.
Combined with the fact that this patch stack means that we can't cancel tasks
after they've been dispatched but before they start running this meant that
some tests were timing out.

The patch also simplifies the rest of that method as I've always found it
confusing.

Attachment #9395249 - Attachment description: Bug 1888429 - Part 1: Remove optimization where we run helper thread tasks on the main thread if they haven't started by the time we wait for them r?jandem → Bug 1888429 - Part 2: Don't run helper thread tasks on the main thread if they've already been dispatched to the thread pool r?jandem

This fixes a deadlock that occurs in an xpcshell test with the rest of the
patches in this stack. It took a while to track this down so I'll explain it
here.

The deadlock happens because of an interaction between several components in
the system. The execution proceeds like this:

  1. The test executes JS. The JS engine queues a TaskController task to JIT
    compile the code.

  2. This is the first off-thread task, so TaskController creates thread pool
    threads. These threads do not start running yet.

  3. The test disables the gecko profiler. Since enable/disable must be atomic,
    the profiler takes the Gecko Profiler mutex.

  4. While holding this mutex, the profiler calls into the JS engine to perform
    engine-specific steps. The engine needs to discards all JIT code so first
    it attempts to cancel the ongoing JIT compilation.

  5. The JS engine waits for the ongoing JIT compilation to finish.

  6. The TaskController thread that would execute the compilation task starts
    executing and registers itself with the profiler, attempting to take the
    profiler mutex already held by the main thread. This deadlocks.

The individual pieces of this seem reasonable to me. It's fair for the profiler
to hold its mutex while enabling/disabling itself. It's fair for the JS engine
to wait for it's off-thread tasks to complete. And it's fair for the thread
pool threads to register themselves with the profiler on startup. However taken
together we have the possibility of deadlock.

To fix this, the patch makes TaskController wait for thread pool threads to be
ready to service tasks (after releasing the profiler lock) before returning
when initializing the thread pool.

Depends on: 1896068
Attachment #9400901 - Attachment is obsolete: true
No longer regressions: 1896537
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68c33310ad20
Part 1: Pass task to run to helper thread dispatch callback r=jandem,mccr8
https://hg.mozilla.org/integration/autoland/rev/5cf3c43ba7e9
Part 1.5: Limit number of dispatched tasks for all task kinds r=jandem
https://hg.mozilla.org/integration/autoland/rev/31790ed29503
Part 2: Don't run helper thread tasks on the main thread if they've already been dispatched to the thread pool r=jandem
https://hg.mozilla.org/integration/autoland/rev/ca0f7caacf8d
Part 3: Refactor helper thread code to make more GlobalHelperThreadState members private r=jandem
https://hg.mozilla.org/integration/autoland/rev/062e83b9f35b
Part 4: Make the internal thread pool dispatch tasks to free threads in a fixed order r=jandem
Regressions: 1899113
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: