Comment 2 for bug 1085613

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

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:
AnalyserQueue trackProgress(TrackPointer, int) and
TrackInfoObject analyserProgress(int)

Both are queued in the event queue so their slots are invoked by the main thread.

The code in your patch emits:
AnalyserQueue updateProgress()
AnalyserQueue trackProgress(int) and
TrackInfoObject analyserProgress(int)

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 progressUpdateInhibitTimer 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::slotTrackLoaded get called? I see no connections.

* How is WOverview::m_trackLoaded different from checking if WOverview::m_pCurrentTrack is not null?

* In DlgPrepare, it looks like slotTrackAnalysisProgress and trackAnalysisFinished are just rebroadcasts so they could just be a SIGNAL to SIGNAL connection instead of having a SLOT.