Re: Big performance problem with QDateTime

Robert Krawitz <rlk-FrUbXkNCsVf2fBVCVOL8/[email protected]> Sun, 20 Sep 2020 17:43:26 -0400
Newsgroups gmane.comp.kde.kimdaba
Message-ID <[email protected]>
On 9/20/20 5:29 PM, Johannes Zarl-Zierl wrote:
> Hi,
> 
> Am Sonntag, 20. September 2020, 17:38:25 CEST schrieb Robert Krawitz:
>>> And adding the ms representation since the epoch of the date and time as a
>>> member variable, along with a boolean one that indicates if the cache
>>> should be recomputed next time it's requested should be trivial to add.
>>> Without breaking the API.
>>>
>>> Thanks for tracking this down :-)
>>
>> I'd like to get the patch in anyway, if you and Johannes don't object.  Even
>> if Qt fixes it, it's going to take a while for the fix there to make it
>> out, and in the interim it's drastically slowing things down here.
> 
> I'll sleep on it and see if we can encapsulate this a little better or make it 
> nicer to read - I fear that by the time Qt catches up and removes the 
> performance bottleneck we have no idea why we convert to seconds since epoch 
> in the first place.
> 
> Apart from that, I concur that we should fix this on our side for now...

Does this comment help?

_______________________________________________
KPhotoAlbum mailing list
[email protected]
https://mail.kdab.com/mailman/listinfo/kphotoalbum
qdatetime-perf-1.patch (text/x-patch, 5 KB)
diff --git a/XMLDB/XMLImageDateCollection.cpp b/XMLDB/XMLImageDateCollection.cpp
index 5fa052da..38bc2c79 100644
--- a/XMLDB/XMLImageDateCollection.cpp
+++ b/XMLDB/XMLImageDateCollection.cpp
@@ -20,19 +20,23 @@
 
 #include <DB/FileNameList.h>
 #include <DB/ImageDB.h>
+#include <stdint.h>
 
 void XMLDB::XMLImageDateCollection::add(const DB::ImageDate &date)
 {
-    m_startIndex.insertMulti(date.start(), date);
+    if (date.start().isValid()) {
+        qint64 startMSecs = date.start().toMSecsSinceEpoch();
+        m_startIndex.insertMulti(startMSecs, date);
+    }
 }
 
 void XMLDB::XMLImageDateCollection::buildIndex()
 {
     StartIndexMap::ConstIterator startSearch = m_startIndex.constBegin();
 #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
-    QDateTime biggestEnd = QDate(1900, 1, 1).startOfDay();
+    qint64 biggestEnd = QDate(1900, 1, 1).startOfDay().toMSecsSinceEpoch();
 #else
-    QDateTime biggestEnd = QDateTime(QDate(1900, 1, 1));
+    qint64 biggestEnd = QDateTime(QDate(1900, 1, 1)).toMSecsSinceEpoch();
 #endif
     for (StartIndexMap::ConstIterator it = m_startIndex.constBegin();
          it != m_startIndex.constEnd();
@@ -42,11 +46,14 @@ void XMLDB::XMLImageDateCollection::buildIndex()
         // have to keep the last pointer as long as we find smaller end-dates.
         // This should be rare as it only occurs if there are images that
         // actually represent a range not just a point in time.
-        if (it.value().end() >= biggestEnd) {
-            biggestEnd = it.value().end();
-            startSearch = it;
+        if (it.value().end().isValid()) {
+            qint64 endMSecs = it.value().end().toMSecsSinceEpoch();
+            if (endMSecs >= biggestEnd) {
+                biggestEnd = endMSecs;
+                startSearch = it;
+            }
+            m_endIndex.insert(endMSecs, startSearch);
         }
-        m_endIndex.insert(it.value().end(), startSearch);
     }
 }
 
@@ -87,11 +94,12 @@ DB::ImageCount XMLDB::XMLImageDateCollection::count(const DB::ImageDate &range)
 
     // We start searching in ranges that overlap our start search range, i.e.
     // where the end-date is higher than our search start.
-    EndIndexMap::Iterator endSearch = m_endIndex.lowerBound(range.start());
+    EndIndexMap::Iterator endSearch = m_endIndex.lowerBound(range.start().toMSecsSinceEpoch());
 
+    qint64 endMSecs = range.end().toMSecsSinceEpoch();
     if (endSearch != m_endIndex.end()) {
         for (StartIndexMap::ConstIterator it = endSearch.value();
-             it != m_startIndex.constEnd() && it.key() < range.end();
+             it != m_startIndex.constEnd() && it.key() < endMSecs;
              ++it) {
             DB::ImageDate::MatchType tp = it.value().isIncludedIn(range);
             switch (tp) {
@@ -117,8 +125,11 @@ QDateTime XMLDB::XMLImageDateCollection::lowerLimit() const
     if (!m_startIndex.empty()) {
         // skip null dates:
         for (StartIndexMap::ConstIterator it = m_startIndex.constBegin(); it != m_startIndex.constEnd(); ++it) {
-            if (it.key().isValid())
-                return it.key();
+            if (it.key() != INT64_MIN) {
+                QDateTime answer;
+                answer.setMSecsSinceEpoch(it.key());
+                return answer;
+            }
         }
     }
 #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
@@ -133,7 +144,9 @@ QDateTime XMLDB::XMLImageDateCollection::upperLimit() const
     if (!m_endIndex.empty()) {
         EndIndexMap::ConstIterator highest = m_endIndex.constEnd();
         --highest;
-        return highest.key();
+        QDateTime answer;
+        answer.setMSecsSinceEpoch(highest.key());
+        return answer;
     }
 #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
     return QDate(2100, 1, 1).startOfDay();
diff --git a/XMLDB/XMLImageDateCollection.h b/XMLDB/XMLImageDateCollection.h
index 1d49e2ea..5eda6eee 100644
--- a/XMLDB/XMLImageDateCollection.h
+++ b/XMLDB/XMLImageDateCollection.h
@@ -40,8 +40,22 @@ public:
     QDateTime upperLimit() const override;
 
 private:
-    typedef QMap<QDateTime, DB::ImageDate> StartIndexMap;
-    typedef QMap<QDateTime, StartIndexMap::ConstIterator> EndIndexMap;
+    // Use qint64 rather than QDateTime because comparison between
+    // QDateTime (via operator<, used by QMap) is very slow and this
+    // leads to substantial delays at startup and when selecting a
+    // very large number of images.
+    //
+    // On a Xeon E3-1505v5, selecting all images from a set of about
+    // 380,000 takes 20 seconds or thereabouts; about 95% of the time
+    // is spent in QDateTime::operator< .  Using qint64 (converting
+    // all of the QDateTime's to qint64's in one shot), it takes about
+    // 2 seconds.  Even so, over 70% of that time is spent in the
+    // conversion to linear milliseconds.
+    //
+    // More background in https://bugreports.qt.io/browse/QTBUG-75585
+    // and https://bugreports.qt.io/browse/QTBUG-41714
+    typedef QMap<qint64, DB::ImageDate> StartIndexMap;
+    typedef QMap<qint64, StartIndexMap::ConstIterator> EndIndexMap;
 
     void add(const DB::ImageDate &);