From 312b72de5f67edf0dff5b04e7cafee479a93a69b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 18 Jan 2022 13:50:26 +0100 Subject: [PATCH 1/6] TrackDAO: Simplify return statements --- src/library/dao/trackdao.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 8cba6a9b8c3..6e8a67dc46e 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -145,7 +145,7 @@ void TrackDAO::finish() { TrackId TrackDAO::getTrackIdByLocation(const QString& location) const { if (location.isEmpty()) { - return TrackId(); + return {}; } QSqlQuery query(m_database); @@ -156,11 +156,11 @@ TrackId TrackDAO::getTrackIdByLocation(const QString& location) const { query.bindValue(":location", location); VERIFY_OR_DEBUG_ASSERT(query.exec()) { LOG_FAILED_QUERY(query); - return TrackId(); + return {}; } if (!query.next()) { qDebug() << "TrackDAO::getTrackId(): Track location not found in library:" << location; - return TrackId(); + return {}; } const auto trackId = TrackId(query.value(query.record().indexOf("id"))); DEBUG_ASSERT(trackId.isValid()); @@ -826,7 +826,7 @@ TrackPointer TrackDAO::addTracksAddFile( qWarning() << "TrackDAO::addTracksAddFile:" << "Unsupported file type" << fileAccess.info().location(); - return TrackPointer(); + return nullptr; } GlobalTrackCacheResolver cacheResolver(fileAccess); @@ -835,7 +835,7 @@ TrackPointer TrackDAO::addTracksAddFile( qWarning() << "TrackDAO::addTracksAddFile:" << "File not found" << fileAccess.info().location(); - return TrackPointer(); + return nullptr; } const TrackId oldTrackId = pTrack->getId(); if (oldTrackId.isValid()) { @@ -856,7 +856,7 @@ TrackPointer TrackDAO::addTracksAddFile( << trackLocation << "are referencing the same file" << fileAccess.info().canonicalLocation(); - return TrackPointer(); + return nullptr; } return pTrack; } @@ -882,7 +882,7 @@ TrackPointer TrackDAO::addTracksAddFile( << "Failed to add track to database" << pTrack->getFileInfo(); // GlobalTrackCache will be unlocked implicitly - return TrackPointer(); + return nullptr; } // The track object has already been initialized with the // database id, but the cache is not aware of this yet. @@ -1329,7 +1329,7 @@ struct ColumnPopulator { TrackPointer TrackDAO::getTrackById(TrackId trackId) const { if (!trackId.isValid()) { - return TrackPointer(); + return nullptr; } // The GlobalTrackCache is only locked while executing the following line. @@ -1590,7 +1590,7 @@ TrackId TrackDAO::getTrackIdByRef( TrackPointer TrackDAO::getTrackByRef( const TrackRef& trackRef) const { if (!trackRef.isValid()) { - return TrackPointer(); + return nullptr; } { GlobalTrackCacheLocker cacheLocker; @@ -1605,7 +1605,7 @@ TrackPointer TrackDAO::getTrackByRef( } if (!trackId.isValid()) { qWarning() << "Track not found:" << trackRef; - return TrackPointer(); + return nullptr; } return getTrackById(trackId); } From ef5e6281813da3d4477848cc82f44b653aaba920 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 18 Jan 2022 13:55:42 +0100 Subject: [PATCH 2/6] TrackDAO: Avoid unnecessary scopes and temporary variables --- src/library/dao/trackdao.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 6e8a67dc46e..543bf669861 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1577,12 +1577,11 @@ TrackId TrackDAO::getTrackIdByRef( if (trackRef.getId().isValid()) { return trackRef.getId(); } - { - GlobalTrackCacheLocker cacheLocker; - const auto pTrack = cacheLocker.lookupTrackByRef(trackRef); - if (pTrack) { - return pTrack->getId(); - } + const auto pTrack = GlobalTrackCacheLocker().lookupTrackByRef(trackRef); + if (pTrack) { + const auto trackId = pTrack->getId(); + DEBUG_ASSERT(trackId.isValid()); + return trackId; } return getTrackIdByLocation(trackRef.getLocation()); } @@ -1592,12 +1591,9 @@ TrackPointer TrackDAO::getTrackByRef( if (!trackRef.isValid()) { return nullptr; } - { - GlobalTrackCacheLocker cacheLocker; - auto pTrack = cacheLocker.lookupTrackByRef(trackRef); - if (pTrack) { - return pTrack; - } + const auto pTrack = GlobalTrackCacheLocker().lookupTrackByRef(trackRef); + if (pTrack) { + return pTrack; } auto trackId = trackRef.getId(); if (!trackId.isValid()) { From 7f39c24551bc49205438f72ce1446fdc8a80ad5c Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 18 Jan 2022 13:57:31 +0100 Subject: [PATCH 3/6] TrackDAO: Replace warning with debug log The caller has to decide if this is actually a problem or not. Logging a warning unconditionally will be confusing for some use cases were this result is actually expected. --- src/library/dao/trackdao.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 543bf669861..7a2462240da 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1600,7 +1600,7 @@ TrackPointer TrackDAO::getTrackByRef( trackId = getTrackIdByLocation(trackRef.getLocation()); } if (!trackId.isValid()) { - qWarning() << "Track not found:" << trackRef; + qDebug() << "Track not found:" << trackRef; return nullptr; } return getTrackById(trackId); From db31296b20f0c822f62eaee295b8ed6a2ef499ec Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 18 Jan 2022 13:58:34 +0100 Subject: [PATCH 4/6] TrackDAO: Clean up and reorder code in saveTrack() --- src/library/dao/trackdao.cpp | 175 +++++++++++++++++++---------------- 1 file changed, 96 insertions(+), 79 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 7a2462240da..dccb6d6c567 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1325,8 +1325,6 @@ struct ColumnPopulator { } // namespace -#define ARRAYLENGTH(x) (sizeof(x) / sizeof(*x)) - TrackPointer TrackDAO::getTrackById(TrackId trackId) const { if (!trackId.isValid()) { return nullptr; @@ -1338,15 +1336,8 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { return pTrack; } - // Accessing the database is a time consuming operation that should not - // be executed with a lock on the GlobalTrackCache. The GlobalTrackCache - // will be locked again after the query has been executed (see below) - // and potential race conditions will be resolved. - ScopedTimer t("TrackDAO::getTrackById"); - QSqlQuery query(m_database); - - ColumnPopulator columns[] = { - // Location must be first. + constexpr ColumnPopulator columns[] = { + // Location must be first and is populated manually! {"track_locations.location", nullptr}, {"artist", setTrackArtist}, {"title", setTrackTitle}, @@ -1404,75 +1395,97 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { {"coverart_digest", nullptr}, {"coverart_hash", nullptr}, }; + constexpr int columnsCount = sizeof(columns) / sizeof(columns[0]); - QString columnsStr; - int columnsSize = 0; - const int columnsCount = ARRAYLENGTH(columns); - for (int i = 0; i < columnsCount; ++i) { - columnsSize += qstrlen(columns[i].name) + 1; - } - columnsStr.reserve(columnsSize); - for (int i = 0; i < columnsCount; ++i) { - if (i > 0) { - columnsStr.append(QChar(',')); - } - columnsStr.append(columns[i].name); - } + // Accessing the database is a time consuming operation that should not + // be executed with a lock on the GlobalTrackCache. The GlobalTrackCache + // will be locked again after the query has been executed (see below) + // and potential race conditions will be resolved. + ScopedTimer t("TrackDAO::getTrackById"); - query.prepare(QString( - "SELECT %1 FROM Library " - "INNER JOIN track_locations ON library.location = track_locations.id " - "WHERE library.id = %2").arg(columnsStr, trackId.toString())); + QSqlRecord queryRecord; + { + QString columnsStr; + int columnsSize = 0; + for (int i = 0; i < columnsCount; ++i) { + columnsSize += qstrlen(columns[i].name) + 1; + } + columnsStr.reserve(columnsSize); + for (int i = 0; i < columnsCount; ++i) { + if (i > 0) { + columnsStr.append(QChar(',')); + } + columnsStr.append(columns[i].name); + } - VERIFY_OR_DEBUG_ASSERT(query.exec()) { - LOG_FAILED_QUERY(query) - << QString("getTrack(%1)").arg(trackId.toString()); - return TrackPointer(); - } - if (!query.next()) { - qDebug() << "Track with id =" << trackId << "not found"; - return TrackPointer(); - } + QSqlQuery query(m_database); + query.prepare(QString( + "SELECT %1 FROM Library " + "INNER JOIN track_locations ON library.location = track_locations.id " + "WHERE library.id = %2") + .arg(columnsStr, trackId.toString())); + VERIFY_OR_DEBUG_ASSERT(query.exec()) { + LOG_FAILED_QUERY(query) + << QString("getTrack(%1)").arg(trackId.toString()); + return nullptr; + } - QSqlRecord queryRecord = query.record(); - int recordCount = queryRecord.count(); - VERIFY_OR_DEBUG_ASSERT(recordCount == columnsCount) { - recordCount = math_min(recordCount, columnsCount); + if (!query.next()) { + qDebug() << "Track with id =" << trackId << "not found"; + return nullptr; + } + queryRecord = query.record(); + // Only a single record is expected + DEBUG_ASSERT(!query.next()); } - // Location is the first column. - const QString trackLocation(queryRecord.value(0).toString()); - - GlobalTrackCacheResolver cacheResolver( - mixxx::FileAccess(mixxx::FileInfo(trackLocation)), trackId); - pTrack = cacheResolver.getTrack(); - if (cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::Hit) { - // Due to race conditions the track might have been reloaded - // from the database in the meantime. In this case we abort - // the operation and simply return the already cached Track - // object which is up-to-date. - DEBUG_ASSERT(pTrack); - return pTrack; - } - if (cacheResolver.getLookupResult() == - GlobalTrackCacheLookupResult::ConflictCanonicalLocation) { - // Reject requests that would otherwise cause a caching caching conflict - // by accessing the same, physical file from multiple tracks concurrently. - DEBUG_ASSERT(!pTrack); - DEBUG_ASSERT(cacheResolver.getTrackRef().hasId()); - DEBUG_ASSERT(cacheResolver.getTrackRef().hasCanonicalLocation()); - kLogger.warning() - << "Failed to load track with id" - << trackId - << "that is referencing the same file" - << cacheResolver.getTrackRef().getCanonicalLocation() - << "as the cached track with id" - << cacheResolver.getTrackRef().getId(); - return pTrack; + { + // Location is the first column. + DEBUG_ASSERT(queryRecord.count() > 0); + const auto trackLocation = queryRecord.value(0).toString(); + const auto fileInfo = mixxx::FileInfo(trackLocation); + const auto fileAccess = mixxx::FileAccess(fileInfo); + const auto cacheResolver = GlobalTrackCacheResolver(fileAccess, trackId); + pTrack = cacheResolver.getTrack(); + switch (cacheResolver.getLookupResult()) { + case GlobalTrackCacheLookupResult::Hit: + // Due to race conditions the track might have been reloaded + // from the database in the meantime. In this case we abort + // the operation and simply return the already cached Track + // object which is up-to-date. + DEBUG_ASSERT(pTrack); + DEBUG_ASSERT(!trackId.isValid() || trackId == pTrack->getId()); + DEBUG_ASSERT(fileInfo == pTrack->getFileInfo()); + return pTrack; + case GlobalTrackCacheLookupResult::Miss: + // An (almost) empty track object + DEBUG_ASSERT(pTrack); + DEBUG_ASSERT(fileInfo == pTrack->getFileInfo()); + DEBUG_ASSERT(!trackId.isValid() || trackId == pTrack->getId()); + // Continue and populate the (almost) empty track object + break; + case GlobalTrackCacheLookupResult::ConflictCanonicalLocation: + // Reject requests that would otherwise cause a caching caching conflict + // by accessing the same, physical file from multiple tracks concurrently. + DEBUG_ASSERT(!pTrack); + DEBUG_ASSERT(cacheResolver.getTrackRef().hasId()); + DEBUG_ASSERT(!trackId.isValid() || trackId == cacheResolver.getTrackRef().getId()); + DEBUG_ASSERT(cacheResolver.getTrackRef().hasCanonicalLocation()); + DEBUG_ASSERT(cacheResolver.getTrackRef().getCanonicalLocation() == + fileInfo.canonicalLocation()); + kLogger.warning() + << "Failed to load track with id" + << trackId + << "that is referencing the same file" + << cacheResolver.getTrackRef().getCanonicalLocation() + << "as the cached track with id" + << cacheResolver.getTrackRef().getId(); + return nullptr; + default: + DEBUG_ASSERT(!"unreachable"); + return nullptr; + } } - DEBUG_ASSERT(cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::Miss); - // The cache will immediately be unlocked to reduce lock contention! - cacheResolver.unlockCache(); // NOTE(uklotzde, 2018-02-06): // pTrack has only the id set and is otherwise empty. It is registered @@ -1486,12 +1499,16 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { // For every column run its populator to fill the track in with the data. bool shouldDirty = false; - for (int i = 0; i < recordCount; ++i) { - TrackPopulatorFn populator = columns[i].populator; - if (populator != nullptr) { - // If any populator says the track should be dirty then we dirty it. - if ((*populator)(queryRecord, i, pTrack.get())) { - shouldDirty = true; + { + int recordCount = queryRecord.count(); + VERIFY_OR_DEBUG_ASSERT(recordCount == columnsCount) { + recordCount = math_min(recordCount, columnsCount); + } + for (int i = 0; i < recordCount; ++i) { + TrackPopulatorFn populator = columns[i].populator; + if (populator) { + // If any populator says the track should be dirty then we dirty it. + shouldDirty |= (*populator)(queryRecord, i, pTrack.get()); } } } From e6eb30c29bda8b6ddee7639a6292ba67f826063e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 18 Jan 2022 17:24:20 +0100 Subject: [PATCH 5/6] TrackDAO: Avoid bitwise or to calculate boolean expression --- src/library/dao/trackdao.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index dccb6d6c567..78ae77cbd6e 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1506,9 +1506,9 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { } for (int i = 0; i < recordCount; ++i) { TrackPopulatorFn populator = columns[i].populator; - if (populator) { + if (populator && (*populator)(queryRecord, i, pTrack.get())) { // If any populator says the track should be dirty then we dirty it. - shouldDirty |= (*populator)(queryRecord, i, pTrack.get()); + shouldDirty = true; } } } From 3b2e5206bb8375eab56e3ad04510608f00e83882 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 18 Jan 2022 17:25:40 +0100 Subject: [PATCH 6/6] TrackDAO: Use std::size to calculate the static array size --- src/library/dao/trackdao.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 78ae77cbd6e..1bd3b5509aa 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1395,7 +1395,7 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { {"coverart_digest", nullptr}, {"coverart_hash", nullptr}, }; - constexpr int columnsCount = sizeof(columns) / sizeof(columns[0]); + constexpr int columnsCount = std::size(columns); // Accessing the database is a time consuming operation that should not // be executed with a lock on the GlobalTrackCache. The GlobalTrackCache