adjust thread priorities

Bug #1270583 reported by RJ Skerry-Ryan
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mixxx
Fix Released
Medium
RJ Skerry-Ryan

Bug Description

In general we should avoid messing around with thread priorities because they can lead to deadlocks in realtime kernels. However, I think that certain classes of tasks should take precedence over others.

Here are our current thread priorities:

PA Callback: TimeCriticalPriority
ControllerManager: HighPriority
HIDReader: HighPriority
BulkReader: HighPriority

CachingReaderWorker: default
EngineWorkerScheduler: default
Main Thread: default
VinylControlProcessor: default
LibraryScanner: default
VSyncThread: default

StatsManager: LowPriority
BrowseThread: LowestPriority
AnalyserQueue: IdlePriority

Proposal:
Background tasks (Analyser, Browse, Stats): LowPriority
GUI tasks (Main thread, VSyncThread): NormalPriority
Live input tasks (VinylControl, HID, Bulk, ControllerManager): HighPriority
Audio Processing tasks (PA Calback, CachingReaderWorker, EngineWorkerScheduler): TimeCriticalPriority

RJ Skerry-Ryan (rryan)
Changed in mixxx:
milestone: none → 1.12.0
Revision history for this message
Daniel Schürmann (daschuer) wrote :

We should look exactly to the effect that happens when a certain task is not scheduled in time.
Here my thoughts without additional investigations:

PA Callback -> Audio Droputs
ControllerManager / HIReader / BlkReader -> delayed control commands (worst case isabsolute position scratch: undesired pitch changes)
CachingReaderWorker -> Nothing, because we read ahead and there is probably still something in cache.
EngineWorkerScheduler > depends on the task (TODO)
Main Thread GUI Thread - > delayed control commands (worst case is Mouse scratch: undesired pitch changes) and waveform stuttering
VinylControlProcessor (does it process the Audiostream? (worst case is absolute position scratch: undesired pitch changes)
LibraryScanner: Nothing
VSyncThread: waveform stuttering (It produces signals for the GUI thread, so no need for a higher priority then the GUI thread.
StatsManager: Nothing?
BrowseThread: Nothing?
AnalyserQueue: Nothing?

From this point of view we do not need TimeCriticalPriority for CachingReaderWorker and EngineWorkerScheduler.
IMHO in general we should only have one TimeCriticalPriority task this has to do all things that has to be done within one Audio buffer size duration but under no circumstances more. If we have more than one CPU, we can parallelize (divide) this single thread for the available CPUs. But we must not have two concurrent tasks at TimeCriticalPriority, see https://bugs.launchpad.net/mixxx/+bug/1203249

It would be also nice to have the GUI task at the same priority than the other input task, but for this it is required to remove all long Slots like db access from it.

Maybe we can also combine all controller polling task to one, to avoid thread changes and random concurrency among them.

CachingReaderWorker: Reading the needed chunk for the next audio Cycle is somehow TimeCriticalPriority, but is a blocking call to HD ....

... Many topics to consider.

Currently Mixxx has ~20 threads! In any case to much for such a low latency audio app :-/ -> a lot of room for improvements.

Changed in mixxx:
status: New → Confirmed
importance: Undecided → Medium
Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Ah, you're right of course. I hadn't had coffee yet :).

The caching reader worker and EWS shouldn't be time critical, but since missing a read request could cause a deck-specific cache miss (silence on the deck, not an xrun) I think it should still be above normal priority.

VinylControlProcessor is analogous to the controller threads with the added caveat that there is a FIFO between it and the PA callback. If it does not read input VC signal from the FIFO often enough then we will buffer over-run and likely cause unstable xwax performance (but the worst case is still inaccurate scratching or undesired pitch changes).

I agree with all your worst-case analyses.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Proposal:

Background tasks (Analyser, Browse, Stats, LibraryScanner): LowPriority

GUI tasks (Main thread, VSyncThread): NormalPriority

Live input tasks (VinylControl, HID, Bulk, ControllerManager): HighPriority
Engine Support tasks (CachingReaderWorker, EngineWorkerScheduler): HighPriority
Sidechain (due to the importance of live broadcasting): HighPriority

Audio Processing tasks (PA Callback): TimeCriticalPriority

Changed in mixxx:
assignee: nobody → RJ Ryan (rryan)
Revision history for this message
Owen Williams (ywwg) wrote :

Do midi and HID really need to be high priority? a millisecond here or there, who's going to notice?

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Since these are all relative (basically nice-values for our threads) it really only matters what they are relative to our other threads.

Based on Daniel's assessment above, incorrect timing from controllers can produce unwanted pitch changes or scratch lagging so I'm on board with all input tasks being in the same priority group. That priority group should be above normal and below time critical so that's why I lumped it into High.

Revision history for this message
Daniel Schürmann (daschuer) wrote :

Sound good for 1.12. Thank you.

Of cause a better solution would be to be in sync with the audio callback, but this might be a future theme.

Revision history for this message
Owen Williams (ywwg) wrote :

SGTM

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :
Changed in mixxx:
status: Confirmed → Fix Committed
RJ Skerry-Ryan (rryan)
Changed in mixxx:
status: Fix Committed → Fix Released
Revision history for this message
Swiftb0y (swiftb0y) wrote :

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/7272

lock status: Metadata changes locked and limited to project staff
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.