TrackCache: Fixes and optimizations (lp1738253/17382271738028)

src/library/basetrackcache.cpp

@@ -145,16 +145,18 @@ void BaseTrackCache::setSearchColumns(const QStringList& columns) {
 }
 
 TrackPointer BaseTrackCache::lookupCachedTrack(TrackId trackId) const {
-    TrackCacheLocker cacheLocker(
-            TrackCache::instance().lookupById(trackId));
-    auto pTrack = cacheLocker.getTrack();
-    if (pTrack && pTrack->isDirty()) {
-        m_dirtyTracks.insert(trackId);
-        return pTrack;
-    } else {
-        m_dirtyTracks.remove(trackId);
-        return TrackPointer();
+    TrackPointer pTrack;
+    if (m_bIsCaching) {
+        pTrack = TrackCache::instance().lookupById(trackId).getTrack();
+        // After obtaining a strong reference of the Track object the lock
+        // on TrackCache has been released instantly to reduce lock contention!
+        if (pTrack && pTrack->isDirty()) {
+            m_dirtyTracks.insert(trackId);
+        } else {
+            m_dirtyTracks.remove(trackId);
+        }
     }
+    return pTrack;
 }
 
 bool BaseTrackCache::updateIndexWithTrackpointer(TrackPointer pTrack) {

src/library/browse/browsethread.cpp

@@ -154,17 +154,16 @@ void BrowseThread::populateModel() {
 
         const QString filepath = fileIt.next();
         {
-            TrackCacheLocker cacheLocker(
-                    TrackCache::instance().lookupOrCreateTemporaryForFile(
-                            filepath, thisPath.token()));
-            auto pTrack = cacheLocker.getTrack();
-            if (!pTrack) {
-                qWarning() << "Skipping inaccessible file"
-                        << filepath;
-                continue;
-            }
-
-            // Import metadata from file
+            TrackPointer pTrack =
+                TrackCache::instance().resolve(
+                        filepath, thisPath.token()).getTrack();
+            // The TrackCache is unlocked instantly even if a new track object
+            // has been created and inserted into the cache. Newly created track
+            // objects will only contain a reference of the corresponding file,
+            // but not any metadata, yet. This reduces lock contention on the
+            // global track cache.
+
+            // Update the track object by (re-)importing metadata from the file
             SoundSourceProxy(pTrack).updateTrackFromSource();
 
             item = new QStandardItem(pTrack->getFileName());

src/library/dao/trackdao.cpp

@@ -188,10 +188,12 @@ bool TrackDAO::trackExistsInDatabase(const QString& absoluteFilePath) {
     return getTrackId(absoluteFilePath).isValid();
 }
 
-void TrackDAO::saveTrack(Track* pTrack) {
-    DEBUG_ASSERT(nullptr != pTrack);
+void TrackDAO::saveTrack(TrackCacheLocker* pCacheLocker, Track* pTrack) {
+    DEBUG_ASSERT(pTrack);
 
     if (pTrack->isDirty()) {
+        qDebug() << "TrackDAO: Saving track" << pTrack->getLocation();
+
         // Write audio meta data, if enabled in the preferences.
         //
         // This must be done before updating the database, because
@@ -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.
+        // Exporting metadata to a file cannot be rolled back if updating
+        // the database fails, so we have to account for inconsistencies
+        // in any case!
+        // Unlocking the cache now reduces lock contention and keeps the
+        // UI as responsive as possible.
+        if (pCacheLocker) {
+            pCacheLocker->unlockCache();
+        }
+
         // Only update the database if the track has already been added!
         const TrackId trackId(pTrack->getId());
         if (trackId.isValid() && updateTrack(pTrack)) {
@@ -635,15 +649,24 @@ TrackPointer TrackDAO::addTracksAddFile(const QFileInfo& fileInfo, bool unremove
 
     TrackCacheResolver cacheResolver(
             TrackCache::instance().resolve(fileInfo));
-    TrackPointer pTrack(cacheResolver.getTrack());
+    const TrackPointer pTrack = cacheResolver.getTrack();
     if (!pTrack) {
         qWarning() << "TrackDAO::addTracksAddFile:"
                 << "File not found"
                 << TrackRef::location(fileInfo);
         return TrackPointer();
     }
+    const TrackId trackId = pTrack->getId();
+    if (trackId.isValid()) {
+        qDebug() << "TrackDAO::addTracksAddFile:"
+                << "Track has already been added to the database"
+                << trackId;
+        return TrackPointer();
+    }
+    // Keep the TrackCache locked until the id of the Track
+    // object is known and has been updated in the cache.
 
-    // Initially load the metadata for the newly created track
+    // Initially (re-)import the metadata for the newly created track
     // from the file.
     SoundSourceProxy(pTrack).updateTrackFromSource();
     if (!pTrack->isMetadataSynchronized()) {
@@ -1241,10 +1264,16 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
         return pTrack;
     }
     if (cacheResolver.getTrackCacheLookupResult() == TrackCacheLookupResult::HIT) {
-        // Must not reload cached tracks from database!
+        // Due to race conditions the track might have been reloaded
+        // from the database. In this case we abort the operation and
+        // simply return the cached Track object.
         return pTrack;
     }
     DEBUG_ASSERT(cacheResolver.getTrackCacheLookupResult() == TrackCacheLookupResult::MISS);
+    DEBUG_ASSERT(pTrack->getId() == trackId);
+    // After the Track object has been cached with an id we can safely
+    // release the lock the cache. This is needed to reduce lock contention.
+    cacheResolver.unlockCache();
 
     // For every column run its populator to fill the track in with the data.
     bool shouldDirty = false;
@@ -1313,10 +1342,6 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
             this, SLOT(slotTrackChanged(Track*)),
             Qt::DirectConnection);
 
-    // Finish the caching operation to release all locks before
-    // emitting any signals.
-    cacheResolver.unlockCache();
-
     // BaseTrackCache cares about track trackDirty/trackClean notifications
     // from TrackDAO that are triggered by the track itself. But the preceding
     // track modifications above have been sent before the TrackDAO has been
@@ -1333,9 +1358,12 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
 TrackPointer TrackDAO::getTrack(TrackId trackId) const {
     //qDebug() << "TrackDAO::getTrack" << QThread::currentThread() << m_database.connectionName();
 
-    TrackCacheLocker cacheLocker(
-            TrackCache::instance().lookupById(trackId));
-    TrackPointer pTrack = cacheLocker.getTrack();
+    // The TrackCache is only locked while executing the following line.
+    TrackPointer pTrack = TrackCache::instance().lookupById(trackId).getTrack();
+    // Accessing the database is a time consuming operation that should
+    // not be executed with a lock on the TrackCache. The TrackCache will
+    // be locked again after the query has been executed and potential
+    // race conditions will be resolved in getTrackFromDB()
     return pTrack ? pTrack : getTrackFromDB(trackId);
 }
 

src/library/dao/trackdao.h

@@ -20,6 +20,7 @@ class AnalysisDao;
 class CueDAO;
 class LibraryHashDAO;
 class TrackCollection;
+class TrackCacheLocker;
 class TrackCacheResolver;
 
 
@@ -133,7 +134,7 @@ class TrackDAO : public QObject, public virtual DAO {
     TrackPointer getTrackFromDB(TrackId trackId) const;
 
     friend class TrackCollection;
-    void saveTrack(Track* pTrack);
+    void saveTrack(TrackCacheLocker* pCacheLocker, Track* pTrack);
     bool updateTrack(Track* pTrack);
 
     QSqlDatabase m_database;

src/library/library.cpp

@@ -422,6 +422,6 @@ void Library::slotSetTrackTableRowHeight(int rowHeight) {
     emit(setTrackTableRowHeight(rowHeight));
 }
 
-void Library::onEvictingTrackFromCache(Track* pTrack) {
-    m_pTrackCollection->saveTrack(pTrack);
+void Library::onEvictingTrackFromCache(TrackCacheLocker* pCacheLocker, Track* pTrack) {
+    m_pTrackCollection->saveTrack(pCacheLocker, pTrack);
 }

src/library/library.h

@@ -81,7 +81,7 @@ class Library: public QObject,
 
     static const int kDefaultRowHeightPx;
 
-    void onEvictingTrackFromCache(Track* pTrack) override;
+    void onEvictingTrackFromCache(TrackCacheLocker* pCacheLocker, Track* pTrack) override;
 
   public slots:
     void slotShowTrackModel(QAbstractItemModel* model);

src/library/trackcollection.cpp

@@ -329,6 +329,6 @@ bool TrackCollection::updateAutoDjCrate(
     return updateCrate(crate);
 }
 
-void TrackCollection::saveTrack(Track* pTrack) {
-    m_trackDao.saveTrack(pTrack);
+void TrackCollection::saveTrack(TrackCacheLocker* pCacheLocker, Track* pTrack) {
+    m_trackDao.saveTrack(pCacheLocker, pTrack);
 }

src/library/trackcollection.h

@@ -80,7 +80,7 @@ class TrackCollection : public QObject,
 
     bool updateAutoDjCrate(CrateId crateId, bool isAutoDjSource);
 
-    void saveTrack(Track* pTrack);
+    void saveTrack(TrackCacheLocker* pCacheLocker, Track* pTrack);
 
   signals:
     void crateInserted(CrateId id);

src/mixxx.cpp

@@ -286,11 +286,17 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) {
             m_pDbConnectionPool,
             m_pPlayerManager,
             m_pRecordingManager);
-    m_pPlayerManager->bindToLibrary(m_pLibrary);
-
-    // Create the singular TrackCache instance
+    // Create the singular TrackCache instance immediately after
+    // the Library has been created and BEFORE binding the
+    // PlayerManager to it!
     TrackCache::createInstance(m_pLibrary);
 
+    // Binding the PlayManager to the Library may already trigger
+    // loading of tracks which requires that the TrackCache has
+    // been created. Otherwise Mixxx might hang when accessing
+    // the uninitialized singleton instance!
+    m_pPlayerManager->bindToLibrary(m_pLibrary);
+
     launchProgress(40);
 
     // Get Music dir

src/test/librarytest.h

@@ -14,8 +14,8 @@ class LibraryTest : public MixxxTest,
     public virtual /*implements*/ TrackCacheEvictor {
 
   public:
-    void onEvictingTrackFromCache(Track* pTrack) override {
-        m_trackCollection.saveTrack(pTrack);
+    void onEvictingTrackFromCache(TrackCacheLocker* pCacheLocker, Track* pTrack) override {
+        m_trackCollection.saveTrack(pCacheLocker, pTrack);
     }
 
   protected: