From 6dabf50781452066e4383497aaf12b3af277e27d Mon Sep 17 00:00:00 2001 From: Ivan Sorokin Date: Wed, 18 Jun 2014 00:43:25 +0400 Subject: [PATCH] Speedup and fix a bug in torrent moving. This commit implements a map where qbittorrent store a state of current torrent movings. This commit speed up torrents moving a bit and also fix a bug when qbittorrent doesn't do cleanup action when a single torrent is moved several times without waiting for a previous move to complete. How it worked before. Libtorrent has a function torrent_handle::move_storage() that allows to move a torrent to a specific directory. This function is asynchorous. It means that this function quits instantaneously and when the actual operation completes the alert 'storage_moved_alert' or 'storage_moved_failed_alert' will be sent. The storage_moved_alert contains a torrent_handle and a new path to where the torrent is moved. During handling of storage_moved_alert, qbittorrent needs not only new path, but also an old path to perform some of cleanup actions (like removing an old folder if it is empty). This was achieved by storing a value named 'previous save path' in TorrentPersistentData. A previous save path is written when move_storage() is issued and is read when storage_moved_alert is received. Problems. This mechanism has two negative aspects: 1. TorrentPersistentData is very slow. As torrent_handle::move_storage() is asynchoronous, TorrentPersistentData is responsible for more that 99.8% of time QTorrentHandle::move_storage(). This percent could be higher when there are lots of torrents and lower when there are few of them. 2. TorrentPersistentData stores only one previous path. But many move_storage()'s could be issued without waiting for previous to complete. Subsequent move_storage()'s overwrites previous save path of a previous move. A fix. The fix is simple. Before issueing move_storage() the oldPath is stored in a special map called 'torrentMoveStates'. When a storage_moved_alert is received the map is consulted and an alert is handled. When user moves torrent when previous moving have not yet finished, the new location is saved in a field 'queuedPath' the same map. When torrent moving is completed (or failed) qbittorrent attemps to perform move again to the queued location. Future direction. This fix removes one slow read and one slow write to TorrentPersistentData on torrent moving, but there is still exists TorrentPersistentData::saveSavePath in handleStorageMovedAlert(), so overall time for UI hang should be reduced only threefold. A speeding up TorrentPersistentData should be addressed in a separate commit. I don't know if I should clean up torrentMoveStates when torrent is deleted. In any case, torrent could be deleted when corresponding alert is in alert queue. So if we decide to clean up torrentMoveStates, then we should not treat receiving alert from unknown torrent as a error. --- src/qtlibtorrent/qbtsession.cpp | 90 +++++++++++++++++++++++------ src/qtlibtorrent/qbtsession.h | 1 + src/qtlibtorrent/qtorrenthandle.cpp | 29 +++++++--- src/torrentpersistentdata.h | 90 ++++++++++++++++++++++++----- 4 files changed, 167 insertions(+), 43 deletions(-) diff --git a/src/qtlibtorrent/qbtsession.cpp b/src/qtlibtorrent/qbtsession.cpp index 97a3b0638..04d18e987 100755 --- a/src/qtlibtorrent/qbtsession.cpp +++ b/src/qtlibtorrent/qbtsession.cpp @@ -80,6 +80,7 @@ //initialize static member variables QHash TorrentTempData::data = QHash(); +QHash TorrentTempData::torrentMoveStates = QHash(); QHash HiddenData::data = QHash(); unsigned int HiddenData::metadata_counter = 0; @@ -2160,6 +2161,9 @@ void QBtSession::handleAlert(libtorrent::alert* a) { case storage_moved_alert::alert_type: handleStorageMovedAlert(static_cast(a)); break; + case storage_moved_failed_alert::alert_type: + handleStorageMovedFailedAlert(static_cast(a)); + break; case metadata_received_alert::alert_type: handleMetadataReceivedAlert(static_cast(a)); break; @@ -2401,22 +2405,73 @@ void QBtSession::handleTorrentDeletedAlert(libtorrent::torrent_deleted_alert* p) void QBtSession::handleStorageMovedAlert(libtorrent::storage_moved_alert* p) { QTorrentHandle h(p->handle); - if (h.is_valid()) { - // Attempt to remove old folder if empty - const QString old_save_path = fsutils::fromNativePath(TorrentPersistentData::getPreviousPath(h.hash())); - const QString new_save_path = fsutils::fromNativePath(misc::toQStringU(p->path.c_str())); - qDebug("Torrent moved from %s to %s", qPrintable(old_save_path), qPrintable(new_save_path)); - QDir old_save_dir(old_save_path); - if (old_save_dir != QDir(defaultSavePath) && old_save_dir != QDir(defaultTempPath)) { - qDebug("Attempting to remove %s", qPrintable(old_save_path)); - QDir().rmpath(old_save_path); - } - if (defaultTempPath.isEmpty() || !new_save_path.startsWith(defaultTempPath)) { - qDebug("Storage has been moved, updating save path to %s", qPrintable(new_save_path)); - TorrentPersistentData::saveSavePath(h.hash(), new_save_path); - } - emit savePathChanged(h); - //h.force_recheck(); + if (!h.is_valid()) { + qWarning("invalid handle received in storage_moved_alert"); + return; + } + + QString hash = h.hash(); + + if (!TorrentTempData::isMoveInProgress(hash)) { + qWarning("unexpected storage_moved_alert received"); + return; + } + + QString new_save_path = fsutils::fromNativePath(misc::toQStringU(p->path.c_str())); + if (new_save_path != TorrentTempData::getNewPath(hash)) { + qWarning("new path received in handleStorageMovedAlert() doesn't match a path in a queue"); + return; + } + + QString oldPath = TorrentTempData::getOldPath(hash); + + qDebug("Torrent is successfully moved from %s to %s", qPrintable(oldPath), qPrintable(new_save_path)); + + // Attempt to remove old folder if empty + QDir old_save_dir(oldPath); + if (old_save_dir != QDir(defaultSavePath) && old_save_dir != QDir(defaultTempPath)) { + qDebug("Attempting to remove %s", qPrintable(oldPath)); + QDir().rmpath(oldPath); + } + if (defaultTempPath.isEmpty() || !new_save_path.startsWith(defaultTempPath)) { + qDebug("Storage has been moved, updating save path to %s", qPrintable(new_save_path)); + TorrentPersistentData::saveSavePath(h.hash(), new_save_path); + } + emit savePathChanged(h); + //h.force_recheck(); + + QString queued = TorrentTempData::getQueuedPath(hash); + if (queued != QString()) { + TorrentTempData::finishMove(hash); + h.move_storage(queued); + } + else { + TorrentTempData::finishMove(hash); + } +} + +void QBtSession::handleStorageMovedFailedAlert(libtorrent::storage_moved_failed_alert* p) { + + QTorrentHandle h(p->handle); + if (!h.is_valid()) { + qWarning("invalid handle received in storage_moved_failed_alert"); + return; + } + + QString hash = h.hash(); + + if (!TorrentTempData::isMoveInProgress(hash)) { + qWarning("unexpected storage_moved_alert received"); + return; + } + + QString queued = TorrentTempData::getQueuedPath(hash); + if (queued != QString()) { + TorrentTempData::finishMove(hash); + h.move_storage(queued); + } + else { + TorrentTempData::finishMove(hash); } } @@ -2968,7 +3023,6 @@ void QBtSession::recoverPersistentData(const QString &hash, const std::vector data) { (*data)["qBt-savePath"] = fsutils::fromNativePath(TorrentPersistentData::getSavePath(hash)).toUtf8().constData(); (*data)["qBt-ratioLimit"] = QString::number(TorrentPersistentData::getRatioLimit(hash)).toUtf8().constData(); - (*data)["qBt-previousSavePath"] = fsutils::fromNativePath(TorrentPersistentData::getPreviousPath(hash)).toUtf8().constData(); (*data)["qBt-label"] = TorrentPersistentData::getLabel(hash).toUtf8().constData(); (*data)["qBt-queuePosition"] = TorrentPersistentData::getPriority(hash); (*data)["qBt-seedStatus"] = (int)TorrentPersistentData::isSeed(hash); diff --git a/src/qtlibtorrent/qbtsession.h b/src/qtlibtorrent/qbtsession.h index d2510275a..fabfb7719 100755 --- a/src/qtlibtorrent/qbtsession.h +++ b/src/qtlibtorrent/qbtsession.h @@ -200,6 +200,7 @@ private: void handleFileRenamedAlert(libtorrent::file_renamed_alert* p); void handleTorrentDeletedAlert(libtorrent::torrent_deleted_alert* p); void handleStorageMovedAlert(libtorrent::storage_moved_alert* p); + void handleStorageMovedFailedAlert(libtorrent::storage_moved_failed_alert* p); void handleMetadataReceivedAlert(libtorrent::metadata_received_alert* p); void handleFileErrorAlert(libtorrent::file_error_alert* p); void handleFileCompletedAlert(libtorrent::file_completed_alert* p); diff --git a/src/qtlibtorrent/qtorrenthandle.cpp b/src/qtlibtorrent/qtorrenthandle.cpp index 8138a3881..1965fecd8 100644 --- a/src/qtlibtorrent/qtorrenthandle.cpp +++ b/src/qtlibtorrent/qtorrenthandle.cpp @@ -445,15 +445,28 @@ void QTorrentHandle::set_tracker_login(const QString& username, const QString& p } void QTorrentHandle::move_storage(const QString& new_path) const { - if (QDir(save_path()) == QDir(new_path)) - return; + QString hashstr = hash(); - TorrentPersistentData::setPreviousSavePath(hash(), save_path()); - // Create destination directory if necessary - // or move_storage() will fail... - QDir().mkpath(new_path); - // Actually move the storage - torrent_handle::move_storage(fsutils::toNativePath(new_path).toUtf8().constData()); + if (TorrentTempData::isMoveInProgress(hashstr)) { + qDebug("enqueue move storage to %s", qPrintable(new_path)); + TorrentTempData::enqueueMove(hashstr, new_path); + } + else { + QString old_path = save_path(); + + qDebug("move storage: %s to %s", qPrintable(old_path), qPrintable(new_path)); + + if (QDir(old_path) == QDir(new_path)) + return; + + TorrentTempData::startMove(hashstr, old_path, new_path); + + // Create destination directory if necessary + // or move_storage() will fail... + QDir().mkpath(new_path); + // Actually move the storage + torrent_handle::move_storage(fsutils::toNativePath(new_path).toUtf8().constData()); + } } bool QTorrentHandle::save_torrent_file(const QString& path) const { diff --git a/src/torrentpersistentdata.h b/src/torrentpersistentdata.h index 2bb351b9e..71726f1ed 100644 --- a/src/torrentpersistentdata.h +++ b/src/torrentpersistentdata.h @@ -42,6 +42,20 @@ #include "qinisettings.h" #include +struct TorrentMoveState +{ + TorrentMoveState(QString oldPath, QString newPath) + : oldPath(oldPath) + , newPath(newPath) + {} + + // the moving occurs from oldPath to newPath + // queuedPath is where files should be moved to, when current moving is completed + QString oldPath; + QString newPath; + QString queuedPath; +}; + class TorrentTempData { // This class stores strings w/o modifying separators public: @@ -101,6 +115,65 @@ public: fp = data.value(hash).files_priority; } + static bool isMoveInProgress(const QString &hash) { + return torrentMoveStates.find(hash) != torrentMoveStates.end(); + } + + static void enqueueMove(const QString &hash, const QString &queuedPath) { + QHash::iterator i = torrentMoveStates.find(hash); + if (i == torrentMoveStates.end()) { + Q_ASSERT(false); + return; + } + i->queuedPath = queuedPath; + } + + static void startMove(const QString &hash, const QString &oldPath, const QString& newPath) { + QHash::iterator i = torrentMoveStates.find(hash); + if (i != torrentMoveStates.end()) { + Q_ASSERT(false); + return; + } + + torrentMoveStates.insert(hash, TorrentMoveState(oldPath, newPath)); + } + + static void finishMove(const QString &hash) { + QHash::iterator i = torrentMoveStates.find(hash); + if (i == torrentMoveStates.end()) { + Q_ASSERT(false); + return; + } + torrentMoveStates.erase(i); + } + + static QString getOldPath(const QString &hash) { + QHash::iterator i = torrentMoveStates.find(hash); + if (i == torrentMoveStates.end()) { + Q_ASSERT(false); + return QString(); + } + return i->oldPath; + } + + static QString getNewPath(const QString &hash) { + QHash::iterator i = torrentMoveStates.find(hash); + if (i == torrentMoveStates.end()) { + Q_ASSERT(false); + return QString(); + } + return i->newPath; + } + + static QString getQueuedPath(const QString &hash) { + QHash::iterator i = torrentMoveStates.find(hash); + if (i == torrentMoveStates.end()) { + Q_ASSERT(false); + return QString(); + } + return i->queuedPath; + } + private: struct TorrentData { TorrentData(): sequential(false), seed(false) {} @@ -113,6 +186,7 @@ private: }; static QHash data; + static QHash torrentMoveStates; }; class HiddenData { @@ -240,22 +314,6 @@ public: return data.value("has_error", false).toBool(); } - static void setPreviousSavePath(const QString &hash, const QString &previous_path) { - QIniSettings settings(QString::fromUtf8("qBittorrent"), QString::fromUtf8("qBittorrent-resume")); - QHash all_data = settings.value("torrents").toHash(); - QHash data = all_data.value(hash).toHash(); - data["previous_path"] = previous_path; - all_data[hash] = data; - settings.setValue("torrents", all_data); - } - - static QString getPreviousPath(const QString &hash) { - QIniSettings settings(QString::fromUtf8("qBittorrent"), QString::fromUtf8("qBittorrent-resume")); - const QHash all_data = settings.value("torrents").toHash(); - const QHash data = all_data.value(hash).toHash(); - return data.value("previous_path").toString(); - } - static QDateTime getSeedDate(const QString &hash) { QIniSettings settings(QString::fromUtf8("qBittorrent"), QString::fromUtf8("qBittorrent-resume")); const QHash all_data = settings.value("torrents").toHash();