Skip to content

Commit

Permalink
Merge pull request #4633 from uklotzde/trackdao
Browse files Browse the repository at this point in the history 
TrackDAO: Refactorings from debugging #4628
  • Loading branch information
@daschuer
daschuer authored Jan 18, 2022
2 parents dfb1bb3 + 3b2e520 commit e6207b3
Showing 1 changed file with 114 additions and 101 deletions.
215 changes: 114 additions & 101 deletions src/library/dao/trackdao.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ void TrackDAO::finish() {

TrackId TrackDAO::getTrackIdByLocation(const QString& location) const {
if (location.isEmpty()) {
return TrackId();
return {};
}

QSqlQuery query(m_database);
Expand All @@ -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());
Expand Down Expand Up @@ -826,7 +826,7 @@ TrackPointer TrackDAO::addTracksAddFile(
qWarning() << "TrackDAO::addTracksAddFile:"
<< "Unsupported file type"
<< fileAccess.info().location();
return TrackPointer();
return nullptr;
}

GlobalTrackCacheResolver cacheResolver(fileAccess);
Expand All @@ -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()) {
Expand All @@ -856,7 +856,7 @@ TrackPointer TrackDAO::addTracksAddFile(
<< trackLocation
<< "are referencing the same file"
<< fileAccess.info().canonicalLocation();
return TrackPointer();
return nullptr;
}
return pTrack;
}
Expand All @@ -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.
Expand Down Expand Up @@ -1325,11 +1325,9 @@ struct ColumnPopulator {

} // namespace

#define ARRAYLENGTH(x) (sizeof(x) / sizeof(*x))

TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
if (!trackId.isValid()) {
return TrackPointer();
return nullptr;
}

// The GlobalTrackCache is only locked while executing the following line.
Expand All @@ -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},
Expand Down Expand Up @@ -1404,75 +1395,97 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
{"coverart_digest", nullptr},
{"coverart_hash", nullptr},
};
constexpr int columnsCount = std::size(columns);

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
Expand All @@ -1486,11 +1499,15 @@ 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())) {
{
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 && (*populator)(queryRecord, i, pTrack.get())) {
// If any populator says the track should be dirty then we dirty it.
shouldDirty = true;
}
}
Expand Down Expand Up @@ -1593,35 +1610,31 @@ 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());
}

TrackPointer TrackDAO::getTrackByRef(
const TrackRef& trackRef) const {
if (!trackRef.isValid()) {
return TrackPointer();
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()) {
trackId = getTrackIdByLocation(trackRef.getLocation());
}
if (!trackId.isValid()) {
qWarning() << "Track not found:" << trackRef;
return TrackPointer();
qDebug() << "Track not found:" << trackRef;
return nullptr;
}
return getTrackById(trackId);
}
Expand Down

0 comments on commit e6207b3

Please sign in to comment.