Skip to content
This repository has been archived by the owner on May 10, 2018. It is now read-only.

Commit

Permalink
[LocationCompleter] Don't leak when query icons,
Browse files Browse the repository at this point in the history 
- it seems it's a fix for #1299
  • Loading branch information
@srazi
srazi committed May 30, 2014
1 parent 7a1f4c7 commit 666ecc4
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/lib/navigation/completer/locationcompleterrefreshjob.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,11 @@ void LocationCompleterRefreshJob::runJob()
}

// Load all icons into QImage
QSqlQuery query;
query.prepare(QSL("SELECT icon FROM icons WHERE url LIKE ? LIMIT 1"));
foreach (QStandardItem* item, m_items) {
const QUrl url = item->data(LocationCompleterModel::UrlRole).toUrl();

QSqlQuery query;
query.prepare(QSL("SELECT icon FROM icons WHERE url LIKE ? LIMIT 1"));
query.addBindValue(QString(QL1S("%1%")).arg(QString::fromUtf8(url.toEncoded(QUrl::RemoveFragment))));
QSqlQuery res = SqlDatabase::instance()->exec(query);

Expand Down

11 comments on commit 666ecc4

@nowrep
Copy link
Member

@nowrep nowrep commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has nothing to do with mentioned issue. The LocationCompleterRefreshJob runs on a separate thread, so the small performance gain has no impact on user experience at all.
Not to mention, the mentioned issue is for v1.6 branch, which don't have this class.

Also, where is the leak in this code?

@pejakm
Copy link
Member

@pejakm pejakm commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit made my location completer faster 4-5 times, if not even more than that.

@pejakm
Copy link
Member

@pejakm pejakm commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I exaggerated a little bit, but it really is noticeable faster.

@pejakm
Copy link
Member

@pejakm pejakm commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What icons?

@nowrep
Copy link
Member

@nowrep nowrep commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In completer. That's because this commit is just wrong.

@srazi
Copy link
Member Author

@srazi srazi commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very sorry for any inconvenience.

Some things mislead me about this patch:

  • I always use QSqlQuery::prepare in this way with QSqlQuery::bindValue and it works correctly, because of this when I wrote this patch I didn't test it deeply, after a few hours I found that QSqlQuery::addBindValue doesn't work like QSqlQury::bindValue and indeed improvement is because of unsuccessful queries. :-D
  • With big loops it differs (initialize QSqlQuery object and prepare it once not each time) but here max iteration is 20+10=30 that is very small, and thus, you are right there is no leak. :)

What is the problem that I incorrectly try to fix it by this patch?
Loading icons on medium size database is very slow for example My profile:
I have ~7000 icon records:
A set of queries that has 29 items that 10 of them have icon needs 28 seconds to finish!!!

Fortunately I found the correct solution:
The problem is when a url contains of one or more %. Also I added a cache for icons for more speedup.

diff --git a/src/lib/navigation/completer/locationcompleterrefreshjob.cpp b/src/lib/navigation/completer/locationcompleterrefreshjob.cpp
index b69c8ec..03b5e6f 100644
--- a/src/lib/navigation/completer/locationcompleterrefreshjob.cpp
+++ b/src/lib/navigation/completer/locationcompleterrefreshjob.cpp
@@ -30,6 +30,7 @@
 #else
 #include <QtConcurrentRun>
 #endif
+QHash<QString, QByteArray> LocationCompleterRefreshJob::m_iconHash;

 LocationCompleterRefreshJob::LocationCompleterRefreshJob(const QString &searchString)
     : QObject()
@@ -99,12 +100,23 @@ void LocationCompleterRefreshJob::runJob()
     query.prepare(QSL("SELECT icon FROM icons WHERE url LIKE ? LIMIT 1"));
     foreach (QStandardItem* item, m_items) {
         const QUrl url = item->data(LocationCompleterModel::UrlRole).toUrl();
+        QString urlString = QString::fromUtf8(url.toEncoded(QUrl::RemoveFragment | QUrl::RemoveQuery | QUrl::RemovePath));
+        urlString.remove(urlString.indexOf("%"), urlString.size());

-        query.addBindValue(QString(QL1S("%1%")).arg(QString::fromUtf8(url.toEncoded(QUrl::RemoveFragment))));
-        QSqlQuery res = SqlDatabase::instance()->exec(query);
-
-        if (res.next()) {
-            item->setData(QImage::fromData(res.value(0).toByteArray()), LocationCompleterModel::ImageRole);
+        if (m_iconHash.contains(urlString)) {
+            item->setData(QImage::fromData(m_iconHash.value(urlString)), LocationCompleterModel::ImageRole);
+        }
+        else {
+            query.bindValue(0, QString(QL1S("%1%")).arg(urlString));
+            QSqlQuery res = SqlDatabase::instance()->exec(query);
+            if (res.next()) {
+                const QByteArray &iconData = res.value(0).toByteArray();
+                m_iconHash.insert(urlString, iconData);
+                item->setData(QImage::fromData(iconData), LocationCompleterModel::ImageRole);
+            }
+            else {
+                m_iconHash.insert(urlString, QByteArray());
+            }
         }
     }

diff --git a/src/lib/navigation/completer/locationcompleterrefreshjob.h b/src/lib/navigation/completer/locationcompleterrefreshjob.h
index b7d4e67..b08cba1 100644
--- a/src/lib/navigation/completer/locationcompleterrefreshjob.h
+++ b/src/lib/navigation/completer/locationcompleterrefreshjob.h
@@ -63,6 +63,7 @@ private:
     QString m_domainCompletion;
     QList<QStandardItem*> m_items;
     QFutureWatcher<void>* m_watcher;
+    static QHash<QString, QByteArray> m_iconHash;
 };

 #endif // LOCATIONCOMPLETERREFRESHJOB_H

@nowrep
Copy link
Member

@nowrep nowrep commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would work if you change the addBindValue to just bindValue(0, ...).
To the performance issue, searching in icons.url should be fast enough because of index on this column (no matter how much icons you have in database).

Edit: Hmm, you are right. It takes a lot of time for a SELECT * FROM icons WHERE url LIKE %.com% query ... I wonder why?
Edit2: However, I get no performance issues directly in QupZilla (with 5500 icons in db)

@srazi
Copy link
Member Author

@srazi srazi commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit2: However, I get no performance issues directly in QupZilla (with 5500 icons in db)
Well, query like ... LIKE %.com% doesn't occur in QupZilla, but complex queries like this one:
... LIKE %http://pozh.org/337-%D8%AA%D9%88%D8%B3%D8%B9%D9%87-%D9%BE%D8%A7%DB%8C%DA%AF%D8%A7%D9%87%E2%80%8C%D8%AF%D8%A7%D8%AF%D9%87-%D8%B3%D8%A7%D8%BA%D8%B1/% that takes a lot of time(indeed I mean hang here for my profile) occurs.

Edit: That I fixed it by these lines in above patch:

+        QString urlString = QString::fromUtf8(url.toEncoded(QUrl::RemoveFragment | QUrl::RemoveQuery | QUrl::RemovePath));
+        urlString.remove(urlString.indexOf("%"), urlString.size());

@nowrep
Copy link
Member

@nowrep nowrep commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's not a correct fix. It should be escaped (not sure how it's done in sqlite).

@pejakm
Copy link
Member

@pejakm pejakm commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can you tell how many icons do you have in db? My database is 21 MB, perhaps it is enough for performance testing?

@srazi
Copy link
Member Author

@srazi srazi commented on 666ecc4 May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pejakm You can use this query SELECT count() FROM icons or use a database browser like this one.

Please sign in to comment.