-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multithreaded analysis #1069
Multithreaded analysis #1069
Conversation
build/depends.py
Outdated
] | ||
if qt5: | ||
qt_imageformats.extend(['qdds', 'qicns', 'qjp2', 'qwbmp', 'qwebp']) | ||
qt_imageformats.extend(['qdds', 'qicns', 'qwbmp', 'qwebp']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: These two changes were done for compatibility when building with QT5. If needed, they can be restored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR.
Here the fist review comments.
src/analyzer/analyzerbeats.cpp
Outdated
@@ -38,7 +39,7 @@ bool AnalyzerBeats::initialize(TrackPointer tio, int sampleRate, int totalSample | |||
return false; | |||
} | |||
|
|||
bool bPreferencesBeatDetectionEnabled = static_cast<bool>( | |||
bool bPreferencesBeatDetectionEnabled = m_batch || static_cast<bool>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does "m_batch" overrides the preferences option?
Could it be renamed to "forceBeatDetectionEnabled"?
The name bPreferencesBeatDetectionEnabled does not fit anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know either why such option existed. I simply changed it to a constructor parameter rather than changing it on analysisfeature. I am open to talk about the need or meaning of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok than I think m_batch should become m_forceBeatDetection and bPreferencesBeatDetectionEnabled should become beatDetectionEnabled.
src/analyzer/analyzermanager.cpp
Outdated
#include "util/timer.h" | ||
#include "util/trace.h" | ||
|
||
AnalyzerManager* AnalyzerManager::m_pAnalyzerManager = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the new nullprt keyword
src/analyzer/analyzermanager.cpp
Outdated
AnalyzerManager& AnalyzerManager::getInstance(UserSettingsPointer pConfig) { | ||
if (!m_pAnalyzerManager) { | ||
// There exists only one UserSettingsPointer in the app, so it doens't matter | ||
// if we only assign it once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the comment.
It is untypical to pass a parameter in the getInstance() call.
We have decided not to have the user pointer as a singleton. Here it becomes one implicit.
I think the best here would be to either make UserSettingsPointer a singleton as well or remove the sigleton nature from AnalyzerManager;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation needs that there exists only one manager so that it can control the number of threads in execution. Since it is used from two different places (analysisfeature and playermanager), it cannot be an instance on those classes either.
Still, I think that I was wrong in using the getInstance() naming/model and should have been a MixxxMainWindow instance, where I have UserSettingsPointer.
I was busy enough trying to understand QThread and signals so I took the easiest path. Now I am open to change this if you give me the correct guidelines to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this not become a singleton and instead pass it throughout to all the places it needs to go. Once #941 is in, there will be a better place for it in the Core class but for now it can go in the main window (src/mixxx.cpp).
src/analyzer/analyzermanager.cpp
Outdated
int maxThreads = m_pAnalyzerManager->m_pConfig->getValue<int>(ConfigKey("[Library]", "MaxAnalysisThreads")); | ||
int ideal = QThread::idealThreadCount(); | ||
if (QThread::idealThreadCount() < 1) { | ||
ideal = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also print a debug message.
src/analyzer/analyzermanager.cpp
Outdated
m_pAnalyzerManager = new AnalyzerManager(pConfig); | ||
} | ||
int maxThreads = m_pAnalyzerManager->m_pConfig->getValue<int>(ConfigKey("[Library]", "MaxAnalysisThreads")); | ||
int ideal = QThread::idealThreadCount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideal can be moved into the if branch below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. yup... Initially I was using the ideal in the if, instead of the hardcoded 32, but then i had the use case where i get -1 for ideal and i wasn't sure how to handle that.
Now that I think about it, i believe i will need to change this again, because now it does not work as I've explained above (it does not prevent opening 8 threads on a 4thread CPU)
src/analyzer/analyzermanager.cpp
Outdated
// if we only assign it once. | ||
m_pAnalyzerManager = new AnalyzerManager(pConfig); | ||
} | ||
int maxThreads = m_pAnalyzerManager->m_pConfig->getValue<int>(ConfigKey("[Library]", "MaxAnalysisThreads")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the use case, that some users (incuding me) have shared preferences on different machines.
I also think that this options exposes to much interns to the user. Is this option required at all?
Can't we just pick the ideal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the setting was supposed to be a safeguard, and still, there are more safeguards that should control your use case:
Concretely, giving the user the option to control the number of threads allows a way to reduce the usage of resources in case that the machine is not able to keep with the desired audio latency.
Also, the idealthreadcount should work on most places, but its API suggests that it can return -1 if it cannot detect it (i guess that's more for mobile platforms or some OS where the information cannot be retreived).
Finally, on the AnalyzerManager, there is a safety measure where it checks that the idealthreadcount is sane for the machine in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid the average user does not even know what threads are. If we found out during testing that we have an performance issue, it is our job to tweak solve it.
Is there a use case to pick single thread analysis? If yes, a boolean option might be a better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had any issue. I've tested it on an i7 (4 cores, 8 threads), a core 2 duo (2 cores, 2 threads), and a virtual machine with 2 cores assigned. None of them gave me issues when analyzing during playback using the idealthreadcount. On the virtual machine i needed to have a latency of 90ms, but that's a bug of virtualbox.
As for the user point of view, I've designed it in a way that the user can only reduce from the ideal thread count, so the only harm that he can cause is to take longer to analyze than what's needed (which is, in fact, the current situation).
In other words, if we get reports of users having more problems due to the multithreaded analysis, we have the option to tell them to reduce this setting until they are satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, single core CPUs are rather rare nowadays.
As for the user point of view, I've designed it in a way that the user can only reduce from the ideal thread count, so the only harm that he can cause is to take longer to analyze than what's needed (which is, in fact, the current situation).
In other words, if we get reports of users having more problems due to the multithreaded analysis, we have the option to tell them to reduce this setting until they are satisfied.
That makes sense. This could be useful, for example, if a user wants to analyze their library but also keep 1 CPU core unused by Mixxx for doing something else with their computer while Mixxx is analyzing.
src/analyzer/analyzermanager.cpp
Outdated
stop(true); | ||
} | ||
|
||
bool AnalyzerManager::isActive(bool includeForeground) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isActive(true) is hard to read at the caller.
how about add two calls? isActive() and isBackgroundWorkerActive()
How does it work if you:
does it keep the 8 thread preference on the new computer or does it lower it to 2 as the number of cores on the new computer ? |
The code as it is currently in this PR does not adapt it as per my comment in the AnalyzerManager::getInstance() method. Said that, I am able to test your scenario (the desktop is 8thread and the laptop is 2 thread, and i can even test that on a virtual machine too) |
src/analyzer/analyzermanager.cpp
Outdated
emit(trackFinished(m_backgroundWorkers.size() + m_batchTrackQueue.size() - 1)); | ||
} | ||
//TODO: Which is the consequence of not calling clear? | ||
#if QT_VERSION < QT_VERSION_CHECK(5, 0, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use reset(). This conditional compilation is no longer necessary since we introduced a dedicated class for TrackPointer that handles the differences between QSharedPointer and std::shared_pointer internally.
src/analyzer/analyzermanager.cpp
Outdated
|
||
//This is used when the maxThreads change. Extra workers are paused until active workers end. | ||
//Then, those are terminated and the paused workers are resumed. | ||
TrackPointer track = TrackPointer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" = TrackPointer();" is redundant.
src/analyzer/analyzerqueue.cpp
Outdated
@@ -409,7 +414,11 @@ void AnalyzerQueue::slotUpdateProgress() { | |||
if (m_progressInfo.current_track) { | |||
m_progressInfo.current_track->setAnalyzerProgress( | |||
m_progressInfo.track_progress); | |||
m_progressInfo.current_track.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
src/analyzer/analyzerworker.cpp
Outdated
|
||
void AnalyzerWorker::nextTrack(TrackPointer newTrack) { | ||
m_qm.lock(); | ||
#if QT_VERSION < QT_VERSION_CHECK(5, 0, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary to reset() the smart pointer before assigning it
src/analyzer/analyzerworker.cpp
Outdated
} | ||
|
||
void AnalyzerWorker::nextTrack(TrackPointer newTrack) { | ||
m_qm.lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock()/unlock() is not exception safe and fragile for refactorings, always prefer QMutexLocker
src/analyzer/analyzerworker.cpp
Outdated
} | ||
|
||
void AnalyzerWorker::endProcess() { | ||
m_exit = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsafe in multithreaded environment, even if it is only a bool. Either use atomics or protect access by a mutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_exit is set to false in AnalyzerWorker constructor, and set to true just once when the manager calls this method from its thread (the UI thread). The workers never modify this value, only check which value it has. It is different than the case with pause, where the workers modify it.
I can put it inside the mutex, but that won't change anything because the workers don't block on mutexes to check the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make it a QAtomicInt since that's what we use for exit-flags elsewhere.
As written the compiler can cache access to m_exit in doAnalysis since it can see that nothing mutates it. Using atomics prevents that, so it will always read a fresh copy from memory.
src/analyzer/analyzermanager.cpp
Outdated
|
||
int maxThreads = m_pConfig->getValue<int>(ConfigKey("[Library]", "MaxAnalysisThreads")); | ||
int ideal = QThread::idealThreadCount(); | ||
if (QThread::idealThreadCount() < 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have test data, that the QT idealThreadCount actually is ideal in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If idealthreadcount is -1 (or generally, less than 1), then the program can do two things:
a)Accept what the user configured in the configuration
b)Use only one thread.
I do "b" if the configured value does not seem sane, and do a if it seems sane.
On the preferences dialog, there is a code similar to this, but with the difference that, if idealthreadcount is -1 ( < 1), the user can select between 1 and 8, and the default value is 1.
src/analyzer/analyzermanager.cpp
Outdated
m_backgroundWorkers.append(worker); | ||
} | ||
else { | ||
m_foregroundWorkers.append(worker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes a forground worker a "forground" worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is "priorityWorkers"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bad naming from my side, but I couldn't think of a better one at that time.
Strictly speaking, everything is run in background (i.e. the user does not need to wait for any of them to stop).
background, in this case, means that they are used for the job of the analysis queue (analysis feature of the library), while foreground means that they run an analysis by an immediate action of the user (just loaded a track).
Maybe I should have used worker and priorityworker. Take in mind that the priority is not the only difference, since there are some Mixxx settings that affect them in a different way. (namely the waveform creation and the bpm analysis)
src/analyzer/analyzermanager.cpp
Outdated
m_MaxThreads=threads; | ||
} | ||
|
||
AnalyzerWorker* AnalyzerManager::createNewWorker(bool batchJob) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this: bool priority
Some thought without verifying: The code Is a bit hard to understand / complicated with all the lists. Did you consider to use QThreadPool? I think creating and deleting threads is an expensive operation. Can't we just create and suspend the ideal thread count at Mixxx start and don't delete them before Mixxx shutdown? |
I've tried to make it clearer where I could, and adding comments to explain why certain things are done, but it's more complex than how I initially thought it. I investigated the different ways to work with multithreading that the QT toolkit offers, from QThread to QTConcurrent, including the QThreadPool. With the current implementation, we control the maximum number of threads, we can priorize tracks (and immediately pause non-priorized tracks), and only have maxThread threads amd workers, and 5*maxTheads analyzers. The non-priority workers (with their analyzers) and threads are kept until the queue is empty. Oh. i forgot... about moving workers to other threads. Well, that's exactly what it is done when they are created (They are moved to the new thread). What I am not so sure is if they can be reallocated, or in which conditions they can. |
…ded QMutexLocker, made m_exit QAtomicInt
src/analyzer/analyzermanager.cpp
Outdated
} | ||
} | ||
if (maxThreads <= 0 || maxThreads > ideal) { | ||
qWarning() << "maxThreads value is incorrect. Changing it to " << ideal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also print the invalid value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not add issue a warning if the Mixxx starts the first time with this new preference option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will print that too.
About the initial value, I don't know where first-time values are set up. Could you enlighten me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work:
int maxThreads = m_pConfig->getValue<int>(ConfigKey("[Library]", "MaxAnalysisThreads"), ideal);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, Yup, that would work too, although I meant where is the configuration initizalized for the first time, so that I could assign the ideal value to it at that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With exact this command. If "[Library]", "MaxAnalysisThreads" is not set ideal is returned.
I have just tested this and it is really a huge performance gain! Here some issues from my test:
|
|
||
void AnalyzerWorker::nextTrack(TrackPointer newTrack) { | ||
QMutexLocker locker(&m_qm); | ||
m_currentTrack = newTrack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this called by a different thread? I think it is a race condition since the worker does not assume that the m_currentTrack can be changed under it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is called from the AnalyzerManager thread, but it is not a race condition because this is called only when the analyzerWorker has requested it, and it sleeps on a wait signal until it is waken up. (see lines 196 to 200 in AnalyzerWorker.cpp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, but this is not obvious, since we have also the Resume() and EndProcess() slot, which are able to start the thread before the waitingForNextTrack() is received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be an error to do resume on a worker that is waiting for a track. Also, I don't expect workers to be managed outside of AnalyzerManager, so basically the manager is the only responsible of interacting with them and is responsible of understanding their state.
calling EndProcess doesn't matter much, since it simply changes the exit flag. (it doesn't affect the thread state at that point).
return total > 0; | ||
} | ||
bool AnalyzerManager::isDefaultQueueActive() { | ||
int total = m_defaultWorkers.size() + m_pausedWorkers.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is + here correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, sorry for the noise.
Unfortunately I expect many conflicts with my PR: The database connection handling was so badly designed and unsafe that it needed a major rework. In this context I also fixed many issues in AnalyzerQueue including thread-safety, VAMP initialization, class design, and usage of standard C++ features. @daschuer FYI |
So what do you consider to do from here on? I was going to comment on your PR about my changes in the database connection in this PR, but I thought it unnecessary.. (more so after dauscher asked to separate the PR in multiple pieces) I'm not sure what you want to do... it is obvious that after your PR is merged, additional work will need to be done here because not all changes apply. Is there a way for us to work this in cojunction? I tend to be on #mixxx, if that helps. |
I just wanted to keep everyone updated. Especially you who invested a lot of work in this area. Hopefully we can migrate the improvements from my reworked AnalyzerQueue to your AnalyzerWorker. Opening (and closing) database connections for new threads will become super easy and safe. You even do not need to pass the connections around manually, but instead may access them at any time safely through the pool. The database connection gets closed automatically when the thread exits as a last resort. But we even have double safety with the DbConnectionPool::ThreadLocalScope that allows the owning thread to implicitly close the connection by itself before the destruction is enforced by QThreadStorage. Regarding the initialization: The std::once_flag is superior in that it keeps the code together instead of spreading it into unrelated files. Locality is a code quality feature ;) |
Re VAMP: |
@daschuer @uklotzde @sblaisot I haven't tested it much, and in fact, it crashed on scanning a few times (I could launch the debugger and it was giving problems on a QPen, weird. On another thread seemed to be related to the IIR filter used in the analyzerwaveform, which seems more logic as a point for crash..), but later I could finish all scans. (When it crashed, i didn't have any song loaded, but when worked, I had already loaded a song in a player. Not sure if this could help understand the crash). At least I worked out the main merging problems. |
After a few rebuildings and a bit more testing, the crashes might have been a bad compile. I also had problems with the waveform renderer that got fixed with a completely clean rebuild. |
src/analyzer/analyzermanager.cpp
Outdated
|
||
void AnalyzerManager::stop(bool shutdown) { | ||
m_defaultTrackQueue.clear(); | ||
QListIterator<AnalyzerWorker*> it(m_defaultWorkers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Range based for loops more compact and easier to grasp than while loops with subsequently numbered local variables.
@@ -195,6 +226,21 @@ void DlgPrefLibrary::slotUpdate() { | |||
m_iOriginalTrackTableRowHeight = m_pLibrary->getTrackTableRowHeight(); | |||
spinBoxRowHeight->setValue(m_iOriginalTrackTableRowHeight); | |||
setLibraryFont(m_originalTrackTableFont); | |||
|
|||
m_iOriginalMaxThreads = m_pconfig->getValue<int>(ConfigKey("[Library]", "MaxAnalysisThreads")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for calculating this number is duplicated. The preference dialogs should be dumb and should stay in sync with the corresponding components. If you change this code in one place you will most likely forget to update the other location.
Ok, this time I believe we got it right. (Other than developing a better implementation of how to maintain the running threads). I saw this on the logs, but I assume this is the expected behaviour: Debug [LibraryScanner 1]: DbConnection - Closing database connection: "MIXXX-2" QSqlDatabase(driver=""QSQLITE"", database=""C:/Users/jazet/AppData/Local/Mixxx/mixxxdb.sqlite"", host=""localhost"", port=-1, user=""mixxx"", open=true) |
@JosepMaJAZ Thanks for reminding me about this warning! No, we should actually fix this before the release in the single-threaded version. Maybe I have already an idea why this is happening... |
Warning should be fixed: #1394 |
This is superceded by #1413, right? Please reopen it if not. |
Implementation of multithreaded track analysis (track by track).
Features:
A small improvement has been done on the bpm analysis, where the BPM configuration setting was temporarily altered before. Now this isn't needed. A similar improvement has been done for waveform analysis