Analysis Revamp #1624
Merged
Analysis Revamp #1624
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I just resolved the small merge conflict between 2.1 and master. This rebases cleanly on master. |
This was referenced Apr 18, 2018
Closed
|
There are some merge conflicts between this and the library redesign branch. Is this still a candidate for 2.2 or should we push it back to 2.3? |
|
This work is finished and has proven to be stable for me. I will not rebase it on any feature branch with yet unresolved issues. |
|
Okay, let's push it back to 2.3 then. |
3 tasks
|
@daschuer what are your thoughts about this? Could we merge it for 2.2? |
|
I will have no time to review this before beta. |
daschuer
requested changes
Nov 12, 2018
daschuer
reviewed
Nov 13, 2018
…537_analysis_revamp
|
There is a deadlock deleting a track from GlobalTrackCache if batch analysis is running while Mixxx is shutting down. I can reproduce this every time. If I load an unanalyzed track to a deck then immediately quit Mixxx, there is no deadlock. If I stop batch analysis before quitting Mixxx, there is no deadlock. Log: Backtrace: |
|
The deadlock should be fixed and Mixxx should shutdown cleanly even when executing an ad-hoc analysis (PlayerManager) or a batch analysis (AnalysisFeature). The shutdown procedure is still a weak point in our code with all the runtime dependencies between various components. |
…537_analysis_revamp
Be-ing
reviewed
Dec 28, 2018
|
I confirm the deadlock on shutdown when batch analyzing is fixed. I cannot reproduce the issue with the audio that @WaylonR reported. I'll let my computer finish analyzing ~4000 tracks and if that goes fine, I think this is ready to merge.
#941 is a step in the right direction for cleaning that up. |
13 tasks
|
After 7 hours of analysis, Mixxx crashed when the last track was shown at 95% completion: |
|
This crash is reproducible every time I analyze my whole library. I have 4046 tracks. Backtrace: |
|
The crash occurs even if I only select a single track to analyze for batch analysis. |
|
The Side note: using function pointers for signal & slot connections makes it so much easier to find where signals & slots are used! KDevelop's "Show uses" feature now works as expected! :D |
|
Deleting the vector of workers before the worker threads have been terminated was simply wrong. Reverted the change. This does not affect the shutdown behavior, i.e. no deadlock. |
|
Okay, quitting Mixxx while batch analysis is running and letting batch analysis finish both seem to work now. However, I encountered a bug which I am attempting to reproduce. When I had all tracks analyzed, then tried to reanalyze them all, the progress text showed it was stuck at the last track (everything else continued to work). I have not been able to reproduce this yet since restarting Mixxx from the first time it happened. I suspect it may have been caused by my test database getting into a corrupted state from testing this branch with the previous bugs. I made a fresh database and am letting batch analysis run on my whole library again. |
|
@Be-ing I'm not able to get the progress display stuck at near the end, independent of how many tracks are analyzed. The shutdown process is still a bumpy road and I'm not able to reason about if all threads are finished in time in order to prevent a segfault. As long as we don't experience any segfaults on shutdown I consider the current implementation safe enough. Hopefully we can improve this situation in the future once all components follow a common life cycle and are orchestrated through an asynchronous startup/shutdown protocol. |
|
I can't reproduce that issue with a fresh settings directory. Great work, thank you! |
|
Hi ! |
|
Already available in master for testing, will be released in 2.3.0. |
This was referenced Aug 22, 2022
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The successor of #1413
Related bug reports and feature requests:
https://bugs.launchpad.net/mixxx/+bug/1737537
https://bugs.launchpad.net/mixxx/+bug/1547916
https://bugs.launchpad.net/mixxx/+bug/1738227
https://bugs.launchpad.net/mixxx/+bug/1641153
Only the last commit contains the substantial changes to the analysis framework and finally enables multi-threaded batch analysis. All preceding commits contain smaller fixes and improvements that might qualify for inclusion in a 2.1.x fix release.
Still based on and target towards 2.1, because rebasing on master causes merge conflicts. Will not be rebased on master until all cherries for 2.1 have been picked.