From 1bb9e2eef2dd0053247f100717ff8c214b27ba17 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Wed, 26 Aug 2020 14:00:39 -0500 Subject: [PATCH] refactor: speed up FaviconCache::add() again. (#1402) * refactor: speed up FaviconCache::add() again. Another iteration on FaviconCache::add() since it's still showing up so high in my perf tests. add()'s url argument is now a QString instead of a QUrl, and the class has a private unordered_map that maps QString urls into Keys. Basically, QUrl generation is so expensive that it's worth caching the result to avoid constructing the intermediate QUrl object. Also, ensure that the network reply's `deleteLater()` method is called so that they don't leak, and add 'svg' to the icon list since it's now supported on all major browsers. --- qt/FaviconCache.cc | 107 +++++++++++++++++++++++++++++-------------- qt/FaviconCache.h | 5 +- qt/VariantHelpers.cc | 2 +- 3 files changed, 75 insertions(+), 39 deletions(-) diff --git a/qt/FaviconCache.cc b/qt/FaviconCache.cc index ef5d95386..9a558e81c 100644 --- a/qt/FaviconCache.cc +++ b/qt/FaviconCache.cc @@ -28,13 +28,6 @@ FaviconCache::FaviconCache() : **** ***/ -QString FaviconCache::getCacheDir() -{ - QString const base = QStandardPaths::writableLocation(QStandardPaths::CacheLocation); - - return QDir(base).absoluteFilePath(QStringLiteral("favicons")); -} - namespace { @@ -43,29 +36,64 @@ QPixmap scale(QPixmap pixmap) return pixmap.scaled(FaviconCache::getIconSize(), Qt::KeepAspectRatio, Qt::SmoothTransformation); } +QString getCacheDir() +{ + auto const base = QStandardPaths::writableLocation(QStandardPaths::CacheLocation); + return QDir(base).absoluteFilePath(QStringLiteral("favicons")); } + +QString getScrapedFile() +{ + auto const base = QStandardPaths::writableLocation(QStandardPaths::CacheLocation); + return QDir(base).absoluteFilePath(QStringLiteral("favicons-scraped.txt")); +} + +void markUrlAsScraped(QString const& url_str) +{ + auto skip_file = QFile(getScrapedFile()); + if (skip_file.open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Append)) + { + skip_file.write(url_str.toUtf8()); + skip_file.write("\n"); + } +} + +} // unnamed namespace + void FaviconCache::ensureCacheDirHasBeenScanned() { static bool has_been_scanned = false; - - if (!has_been_scanned) + if (has_been_scanned) { - has_been_scanned = true; + return; + } - QDir cache_dir(getCacheDir()); - cache_dir.mkpath(cache_dir.absolutePath()); + has_been_scanned = true; - QStringList files = cache_dir.entryList(QDir::Files | QDir::Readable); - - for (QString const& file : files) + // remember which hosts we've asked for a favicon so that we + // don't re-ask them every time we start a new session + auto skip_file = QFile(getScrapedFile()); + if (skip_file.open(QIODevice::ReadOnly | QIODevice::Text | QIODevice::ExistingOnly)) + { + while (!skip_file.atEnd()) { - QPixmap pixmap; - pixmap.load(cache_dir.absoluteFilePath(file)); + auto const url = QString::fromUtf8(skip_file.readLine()).trimmed(); + auto const key = getKey(QUrl{ url }); + keys_.insert({ url, key }); + pixmaps_.try_emplace(key); + } + } - if (!pixmap.isNull()) - { - pixmaps_[file] = scale(pixmap); - } + // load the cached favicons + auto cache_dir = QDir(getCacheDir()); + cache_dir.mkpath(cache_dir.absolutePath()); + QStringList const files = cache_dir.entryList(QDir::Files | QDir::Readable); + for (auto const& file : files) + { + QPixmap pixmap(cache_dir.absoluteFilePath(file)); + if (!pixmap.isNull()) + { + pixmaps_[file] = scale(pixmap); } } } @@ -111,26 +139,33 @@ QPixmap FaviconCache::find(Key const& key) return pixmaps_[key]; } -FaviconCache::Key FaviconCache::add(QUrl const& url) +FaviconCache::Key FaviconCache::add(QString const& url_str) { ensureCacheDirHasBeenScanned(); - Key const key = getKey(url); - - if (pixmaps_.count(key) == 0) + // find or add this url's key + auto k_it = keys_.find(url_str); + if (k_it != keys_.end()) { - // add a placholder s.t. we only ping the server once per session - pixmaps_[key] = QPixmap(); + return k_it->second; + } - // try to download the favicon - QString const path = QStringLiteral("http://") + url.host() + QStringLiteral("/favicon."); - QStringList suffixes; - suffixes << QStringLiteral("ico") << QStringLiteral("png") << QStringLiteral("gif") << QStringLiteral("jpg"); + auto const url = QUrl { url_str }; + auto const key = getKey(url); + keys_.insert({ url_str, key }); - for (QString const& suffix : suffixes) - { - nam_->get(QNetworkRequest(path + suffix)); - } + // Try to download a favicon if we don't have one. + // Add a placeholder to prevent repeat downloads. + if (pixmaps_.try_emplace(key).second) + { + markUrlAsScraped(url_str); + + auto const path = QStringLiteral("http://%1/favicon.").arg(url.host()); + nam_->get(QNetworkRequest(path + QStringLiteral("gif"))); + nam_->get(QNetworkRequest(path + QStringLiteral("ico"))); + nam_->get(QNetworkRequest(path + QStringLiteral("jpg"))); + nam_->get(QNetworkRequest(path + QStringLiteral("png"))); + nam_->get(QNetworkRequest(path + QStringLiteral("svg"))); } return key; @@ -165,4 +200,6 @@ void FaviconCache::onRequestFinished(QNetworkReply* reply) // notify listeners emit pixmapReady(key); } + + reply->deleteLater(); } diff --git a/qt/FaviconCache.h b/qt/FaviconCache.h index c12f2679f..fc27d440a 100644 --- a/qt/FaviconCache.h +++ b/qt/FaviconCache.h @@ -35,12 +35,11 @@ public: // returns a cached pixmap, or a NULL pixmap if there's no match in the cache QPixmap find(Key const& key); - QPixmap find(QUrl const& url) { return find(getKey(url)); } static Key getKey(QString const& display_name); // This will emit a signal when (if) the icon becomes ready. - Key add(QUrl const& url); + Key add(QString const& url); static QString getDisplayName(Key const& key); static QSize getIconSize(); @@ -53,9 +52,9 @@ private slots: private: static Key getKey(QUrl const& url); - QString getCacheDir(); void ensureCacheDirHasBeenScanned(); QNetworkAccessManager* nam_ = {}; std::unordered_map pixmaps_; + std::unordered_map keys_; }; diff --git a/qt/VariantHelpers.cc b/qt/VariantHelpers.cc index 2512b25ff..59c85f25f 100644 --- a/qt/VariantHelpers.cc +++ b/qt/VariantHelpers.cc @@ -160,7 +160,7 @@ bool change(TrackerStat& setme, tr_variant const* value) { if (key == TR_KEY_announce) { - setme.favicon_key = qApp->faviconCache().add(QUrl(setme.announce)); + setme.favicon_key = qApp->faviconCache().add(setme.announce); } changed = true;