Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TrackDAO: Refactorings from debugging #4628 #4633

Merged
merged 6 commits into from Jan 18, 2022
Merged

TrackDAO: Refactorings from debugging #4628 #4633

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
312b72d
TrackDAO: Simplify return statements
uklotzde Jan 18, 2022
ef5e628
TrackDAO: Avoid unnecessary scopes and temporary variables
uklotzde Jan 18, 2022
7f39c24
TrackDAO: Replace warning with debug log
uklotzde Jan 18, 2022
db31296
TrackDAO: Clean up and reorder code in saveTrack()
uklotzde Jan 18, 2022
e6eb30c
TrackDAO: Avoid bitwise or to calculate boolean expression
uklotzde Jan 18, 2022
3b2e520
TrackDAO: Use std::size to calculate the static array size
uklotzde Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
217 changes: 115 additions & 102 deletions src/library/dao/trackdao.cpp
View file
Open in desktop
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 = sizeof(columns) / sizeof(columns[0]);
uklotzde marked this conversation as resolved.
Show resolved Hide resolved

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,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());
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -1577,35 +1594,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