TrackCache: Fixes and optimizations (lp1738253/17382271738028) #1415
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
|
@Be-ing Do you have a log excerpt for me? I'm using exactly this combination for tests. |
|
I can't tell at first glance where anything is going wrong, so here is the whole log: |
|
@Be-ing I don't have any recordings, so I'll check that! |
|
Backtrace: |
uklotzde
changed the title
...this conditional behavior has been lost when removing the recent tracks cache from TrackDAO.
...that is triggered when switching between browse views
uklotzde
changed the title
|
Yes, the hanging on startup is solved. |
|
@Be-ing The last commit might fix the hang on startup. |
uklotzde
changed the title
|
Yes, that is fixed. Analysis is still broken. |
...to reduce lock contention and keep the UI responsive
|
Done. I've reduced lock contention on TrackCache as much as possible. We even found and fixed a serious bug of the initialization order on startup! Remaining open issues with the (batch) analysis of tracks do not directly affect the track cache and will be solved separately. |
|
We should include this in the first beta, because I fear that the initialization order bug might cause subsequent freezes or crashes. |
|
Unfortunately this is not reviewed ans so doe not meet our quality standard. |
|
A BIG thanks to @uklotzde for spending so much time into this lately. |
daschuer
reviewed
Dec 25, 2017
| if (!pTrack) { | ||
| qWarning() << "Skipping inaccessible file" | ||
| << filepath; | ||
| continue; |
There was a problem hiding this comment.
Original we skip inaccessible files here. Do you have an idea, when it happens?
Do we now have inaccessible files in the results?
| m_dirtyTracks.remove(trackId); | ||
| return TrackPointer(); | ||
| TrackPointer pTrack; | ||
| if (m_bIsCaching) { |
There was a problem hiding this comment.
Is it correct that this is true for Mixxx tracks only, but not for third party libraries?
Why? It looks like they have their own instance of BaseTrackCache. Is there his kind of legacy?
Can we either unify this to only one instance or inherit form BaseTracKCache as a base class?
I am not sure what makes actually sense here.
|
|
||
| // Import metadata from file | ||
| TrackPointer pTrack = | ||
| TrackCache::instance().resolve( |
There was a problem hiding this comment.
BaseTrackCache uses TrackCache this sounds like swapped class named from legacy.
| @@ -202,6 +204,18 @@ void TrackDAO::saveTrack(Track* pTrack) { | |||
| SoundSourceProxy::exportTrackMetadataBeforeSaving(pTrack); | |||
| } | |||
|
|
|||
| // The track cache can safely be unlocked now that the metadata has | |||
| // been exported to the file. Updating the database is thread-safe | |||
| // an we accept this very small chance of a race condition here. | |||
There was a problem hiding this comment.
A race condition between what? What can happen at worse?
This was referenced Aug 23, 2022
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.
By removing the recent tracks cache from TrackDAO the new TrackCache has now become the central authority for accessing track objects. I didn't notice that lock contention was much too high in some uses cases. Now time spans during which TrackCache is locked should be much shorter!
The most time consuming operation is when entries are evicted and the cache must be kept locked while both updating the database and exporting metadata for safety reasons. I will try to extract the database update from the lock scope, but this might not be possible with the current architecture that closely couples the database and UI layers. Please test if enabling export of track metadata makes a difference for you. I am running my tests at
1011.6 ms latency with metadata export enabled anddidn't notice any drop outs so fardropouts still occur. But these dropouts also occur with older versions where the TrackCache & Export stuff has not been merged. I'm still trying to find out where this regression started.I even discovered a serious bug in TrackCache after changing the access pattern in the browse view. It was caused by a strayed line of code that may have survived a previous merge. Luckily the multitude of debug assertions with consistency checks revealed this bug instantly.
Please note that the "sluggish" response during batch analysis is still not fixed, although it might also profit from these changes.