Analyser queue signal rework
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Mixxx |
Fix Released
|
Medium
|
Daniel Schürmann |
Bug Description
The attached patch is a rework of the signals emitted from Analyser queue patch.
* Removes unused Data from the signals
* It puts only one signal on the Qt event Queue of the GUI thread each progress step, instead of two.
* It ensures that only one process signal is on the GUI Event queue a time. Before the patch, the queue could get poluted with progress updates in case of a long process in the GUI thread.
* No priority inversion
* Small analysis speed up from 7.5 s to 7 s for a 3:30 track on my system.
In general we should prevent to fire timer driven signals between threads, to prevent the GUI Thread from process outdated data when there is high load anyway.
This patch is an improved solution from what was rejected in Bug #1008721.
Changed in mixxx: | |
assignee: | nobody → Daniel Schürmann (daschuer) |
status: | New → In Progress |
importance: | Undecided → Medium |
Changed in mixxx: | |
milestone: | none → 1.11.0 |
Changed in mixxx: | |
status: | Fix Committed → Fix Released |
Thanks for putting the patch together. I am trying to understand why it gives a speed improvement.
The code in lp:mixxx/1.11 emits 2 signals per progress update: TrackPointer, int) and s(int)
AnalyserQueue trackProgress(
TrackInfoObject analyserProgres
Both are queued in the event queue so their slots are invoked by the main thread.
The code in your patch emits: s(int)
AnalyserQueue updateProgress()
AnalyserQueue trackProgress(int) and
TrackInfoObject analyserProgres
But only updateProgress should be queued in the event queue while the latter 2 are direct invoked.
Just making sure I understand the intended benefit of doing it. Correct me if I said something wrong.
The progressUpdateI nhibitTimer delays at least 60ms between progress updates so we are really talking about the difference between 2 queued events every 60 ms and 1 queued event every 60ms.
My question is does this really have a measurable benefit on analysis time and Qt event queue length? The semaphore signalling increases the complexity of this section of code by a lot so I'd rather not do it if we don't get a benefit.
You measured 7.5s -> 7s. Did you measure it many times and take an average or is this just a one-off? I wonder what the average and variance of the distribution of analysis times for a single track is if you analyse it over and over. It's possible the 0.5s speed increase was just noise or within the variance of the measurement. I wonder how we could easily test if this is a statistically significant change.
Also there are many other factors at play. The engine is always calculating while you run these tests so it's adding to the measurement noise. We could make a standalone binary that instantiates the analyser queue and tries to analyze a track over and over.
It seems very odd that doing 1 extra queued event every 60ms is enough to cause 0.5s of overall analysis time difference.
Some other minor code comments:
* How does WOverview: :slotTrackLoade d get called? I see no connections.
* How is WOverview: :m_trackLoaded different from checking if WOverview: :m_pCurrentTrac k is not null?
* In DlgPrepare, it looks like slotTrackAnalys isProgress and trackAnalysisFi nished are just rebroadcasts so they could just be a SIGNAL to SIGNAL connection instead of having a SLOT.