From 86e3d0d816e7a166c14556c742c8d44f30a1fb54 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Tue, 28 Jan 2020 13:53:06 +0800 Subject: [PATCH 1/4] Improve ReverseResolution class * Use QHostAddress type for IP * Avoid double lookup * Use larger cache size --- src/base/net/reverseresolution.cpp | 51 ++++++++++++++------------- src/base/net/reverseresolution.h | 9 ++--- src/gui/properties/peerlistwidget.cpp | 7 ++-- src/gui/properties/peerlistwidget.h | 3 +- 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/src/base/net/reverseresolution.cpp b/src/base/net/reverseresolution.cpp index 89ebf1c6f..1ce8dff72 100644 --- a/src/base/net/reverseresolution.cpp +++ b/src/base/net/reverseresolution.cpp @@ -28,19 +28,19 @@ #include "reverseresolution.h" -#include +#include #include #include -const int CACHE_SIZE = 500; +const int CACHE_SIZE = 2048; using namespace Net; namespace { - bool isUsefulHostName(const QString &hostname, const QString &ip) + bool isUsefulHostName(const QString &hostname, const QHostAddress &ip) { - return (!hostname.isEmpty() && (hostname != ip)); + return (!hostname.isEmpty() && (hostname != ip.toString())); } } @@ -52,37 +52,40 @@ ReverseResolution::ReverseResolution(QObject *parent) ReverseResolution::~ReverseResolution() { - qDebug("Deleting host name resolver..."); } -void ReverseResolution::resolve(const QString &ip) +void ReverseResolution::resolve(const QHostAddress &ip) { - if (m_cache.contains(ip)) { - const QString &hostname = *m_cache.object(ip); - qDebug() << "Resolved host name using cache: " << ip << " -> " << hostname; - if (isUsefulHostName(hostname, ip)) - emit ipResolved(ip, hostname); - } - else { - // Actually resolve the ip - m_lookups.insert(QHostInfo::lookupHost(ip, this, &ReverseResolution::hostResolved), ip); + if (ip.isNull()) + return; + + const QString *hostname = m_cache.object(ip); + if (hostname) { + if (hostname->isEmpty()) + ; // resolution didn't get meaningful results, so do nothing + else + emit ipResolved(ip, *hostname); + return; } + + // do reverse lookup: IP -> hostname + const int lookupId = QHostInfo::lookupHost(ip.toString(), this, &ReverseResolution::hostResolved); + m_lookups.insert(lookupId, ip); } void ReverseResolution::hostResolved(const QHostInfo &host) { - const QString ip = m_lookups.take(host.lookupId()); - Q_ASSERT(!ip.isNull()); + const QHostAddress ip = m_lookups.take(host.lookupId()); - if (host.error() != QHostInfo::NoError) { - qDebug() << "DNS Reverse resolution error: " << host.errorString(); + if (host.error() != QHostInfo::NoError) return; - } const QString hostname = host.hostName(); - - qDebug() << Q_FUNC_INFO << ip << QString("->") << hostname; - m_cache.insert(ip, new QString(hostname)); - if (isUsefulHostName(hostname, ip)) + if (isUsefulHostName(hostname, ip)) { + m_cache.insert(ip, new QString(hostname)); emit ipResolved(ip, hostname); + } + else { + m_cache.insert(ip, new QString()); + } } diff --git a/src/base/net/reverseresolution.h b/src/base/net/reverseresolution.h index 30ffbe14b..875cc2b56 100644 --- a/src/base/net/reverseresolution.h +++ b/src/base/net/reverseresolution.h @@ -32,6 +32,7 @@ #include #include +class QHostAddress; class QHostInfo; class QString; @@ -46,17 +47,17 @@ namespace Net explicit ReverseResolution(QObject *parent = nullptr); ~ReverseResolution(); - void resolve(const QString &ip); + void resolve(const QHostAddress &ip); signals: - void ipResolved(const QString &ip, const QString &hostname); + void ipResolved(const QHostAddress &ip, const QString &hostname); private slots: void hostResolved(const QHostInfo &host); private: - QHash m_lookups; - QCache m_cache; + QHash m_lookups; // + QCache m_cache; // }; } diff --git a/src/gui/properties/peerlistwidget.cpp b/src/gui/properties/peerlistwidget.cpp index fbb70638c..e4d764e65 100644 --- a/src/gui/properties/peerlistwidget.cpp +++ b/src/gui/properties/peerlistwidget.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -405,7 +406,7 @@ void PeerListWidget::updatePeer(const BitTorrent::TorrentHandle *torrent, const m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join('\n'), Qt::ToolTipRole); if (m_resolver) - m_resolver->resolve(peerIp); + m_resolver->resolve(peerEndpoint.address.ip); if (m_resolveCountries) { const QIcon icon = UIThemeManager::instance()->getFlagIcon(peer.country()); @@ -417,10 +418,10 @@ void PeerListWidget::updatePeer(const BitTorrent::TorrentHandle *torrent, const } } -void PeerListWidget::handleResolved(const QString &ip, const QString &hostname) +void PeerListWidget::handleResolved(const QHostAddress &ip, const QString &hostname) const { for (auto iter = m_peerItems.cbegin(); iter != m_peerItems.cend(); ++iter) { - if (iter.key().address.ip.toString() == ip) { + if (iter.key().address.ip == ip) { QStandardItem *item {iter.value()}; item->setData(hostname, Qt::DisplayRole); break; diff --git a/src/gui/properties/peerlistwidget.h b/src/gui/properties/peerlistwidget.h index 314938c75..f57d807e5 100644 --- a/src/gui/properties/peerlistwidget.h +++ b/src/gui/properties/peerlistwidget.h @@ -32,6 +32,7 @@ #include #include +class QHostAddress; class QStandardItem; class QStandardItemModel; @@ -72,7 +73,7 @@ private slots: void banSelectedPeers(); void copySelectedPeers(); void handleSortColumnChanged(int col); - void handleResolved(const QString &ip, const QString &hostname); + void handleResolved(const QHostAddress &ip, const QString &hostname) const; private: void updatePeer(const BitTorrent::TorrentHandle *torrent, const BitTorrent::PeerInfo &peer, bool &isNewPeer); From a2ebd77eac61e0d609847a71a96ba795298c955c Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Wed, 29 Jan 2020 12:39:07 +0800 Subject: [PATCH 2/4] Manually abort lookup on class destruction Some lookup might take longer so instead of waiting them, we abort them manually. --- src/base/net/reverseresolution.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/base/net/reverseresolution.cpp b/src/base/net/reverseresolution.cpp index 1ce8dff72..6c8fd34f6 100644 --- a/src/base/net/reverseresolution.cpp +++ b/src/base/net/reverseresolution.cpp @@ -52,6 +52,9 @@ ReverseResolution::ReverseResolution(QObject *parent) ReverseResolution::~ReverseResolution() { + // abort on-going lookups instead of waiting them + for (auto iter = m_lookups.cbegin(); iter != m_lookups.cend(); ++iter) + QHostInfo::abortHostLookup(iter.key()); } void ReverseResolution::resolve(const QHostAddress &ip) From ff31bb86bc9e49f080220bcaf5a5bceeb99c9e30 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Thu, 30 Jan 2020 13:37:46 +0800 Subject: [PATCH 3/4] Speed up lookup time By adding another variable we can get O(1) lookup time instead of O(n). Fix up 5f415c292ddc2ea13015a2bec42f9f6b710ed732. --- src/gui/properties/peerlistwidget.cpp | 20 ++++++++++++-------- src/gui/properties/peerlistwidget.h | 2 ++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/gui/properties/peerlistwidget.cpp b/src/gui/properties/peerlistwidget.cpp index e4d764e65..cc40c39d5 100644 --- a/src/gui/properties/peerlistwidget.cpp +++ b/src/gui/properties/peerlistwidget.cpp @@ -328,6 +328,7 @@ void PeerListWidget::copySelectedPeers() void PeerListWidget::clear() { m_peerItems.clear(); + m_itemsByIP.clear(); const int nbrows = m_listModel->rowCount(); if (nbrows > 0) m_listModel->removeRows(0, nbrows); @@ -365,7 +366,13 @@ void PeerListWidget::loadPeers(const BitTorrent::TorrentHandle *torrent) // Remove peers that are gone for (const PeerEndpoint &peerEndpoint : asConst(existingPeers)) { - const QStandardItem *item = m_peerItems.take(peerEndpoint); + QStandardItem *item = m_peerItems.take(peerEndpoint); + + QSet &items = m_itemsByIP[peerEndpoint.address.ip]; + items.remove(item); + if (items.isEmpty()) + m_itemsByIP.remove(peerEndpoint.address.ip); + m_listModel->removeRow(item->row()); } } @@ -387,6 +394,7 @@ void PeerListWidget::updatePeer(const BitTorrent::TorrentHandle *torrent, const m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP_HIDDEN), peerIp); itemIter = m_peerItems.insert(peerEndpoint, m_listModel->item(row, PeerListDelegate::IP)); + m_itemsByIP[peerEndpoint.address.ip].insert(itemIter.value()); } const int row = (*itemIter)->row(); @@ -420,13 +428,9 @@ void PeerListWidget::updatePeer(const BitTorrent::TorrentHandle *torrent, const void PeerListWidget::handleResolved(const QHostAddress &ip, const QString &hostname) const { - for (auto iter = m_peerItems.cbegin(); iter != m_peerItems.cend(); ++iter) { - if (iter.key().address.ip == ip) { - QStandardItem *item {iter.value()}; - item->setData(hostname, Qt::DisplayRole); - break; - } - } + const QSet items = m_itemsByIP.value(ip); + for (QStandardItem *item : items) + item->setData(hostname, Qt::DisplayRole); } void PeerListWidget::handleSortColumnChanged(const int col) diff --git a/src/gui/properties/peerlistwidget.h b/src/gui/properties/peerlistwidget.h index f57d807e5..ca05acc82 100644 --- a/src/gui/properties/peerlistwidget.h +++ b/src/gui/properties/peerlistwidget.h @@ -30,6 +30,7 @@ #define PEERLISTWIDGET_H #include +#include #include class QHostAddress; @@ -85,6 +86,7 @@ private: PropertiesWidget *m_properties = nullptr; Net::ReverseResolution *m_resolver = nullptr; QHash m_peerItems; + QHash> m_itemsByIP; // must be kept in sync with `m_peerItems` bool m_resolveCountries; }; From b2ab6c1858d14c8845ed69ea79923a830c807f98 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Thu, 30 Jan 2020 15:04:46 +0800 Subject: [PATCH 4/4] Let ReverseResolution always return/emit a result --- src/base/net/reverseresolution.cpp | 25 +++++++++---------------- src/gui/properties/peerlistwidget.cpp | 3 +++ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/base/net/reverseresolution.cpp b/src/base/net/reverseresolution.cpp index 6c8fd34f6..4d63923b0 100644 --- a/src/base/net/reverseresolution.cpp +++ b/src/base/net/reverseresolution.cpp @@ -59,15 +59,9 @@ ReverseResolution::~ReverseResolution() void ReverseResolution::resolve(const QHostAddress &ip) { - if (ip.isNull()) - return; - const QString *hostname = m_cache.object(ip); if (hostname) { - if (hostname->isEmpty()) - ; // resolution didn't get meaningful results, so do nothing - else - emit ipResolved(ip, *hostname); + emit ipResolved(ip, *hostname); return; } @@ -80,15 +74,14 @@ void ReverseResolution::hostResolved(const QHostInfo &host) { const QHostAddress ip = m_lookups.take(host.lookupId()); - if (host.error() != QHostInfo::NoError) + if (host.error() != QHostInfo::NoError) { + emit ipResolved(ip, {}); return; - - const QString hostname = host.hostName(); - if (isUsefulHostName(hostname, ip)) { - m_cache.insert(ip, new QString(hostname)); - emit ipResolved(ip, hostname); - } - else { - m_cache.insert(ip, new QString()); } + + const QString hostname = isUsefulHostName(host.hostName(), ip) + ? host.hostName() + : QString(); + m_cache.insert(ip, new QString(hostname)); + emit ipResolved(ip, hostname); } diff --git a/src/gui/properties/peerlistwidget.cpp b/src/gui/properties/peerlistwidget.cpp index cc40c39d5..4b3107d66 100644 --- a/src/gui/properties/peerlistwidget.cpp +++ b/src/gui/properties/peerlistwidget.cpp @@ -428,6 +428,9 @@ void PeerListWidget::updatePeer(const BitTorrent::TorrentHandle *torrent, const void PeerListWidget::handleResolved(const QHostAddress &ip, const QString &hostname) const { + if (hostname.isEmpty()) + return; + const QSet items = m_itemsByIP.value(ip); for (QStandardItem *item : items) item->setData(hostname, Qt::DisplayRole);