Edid parser: fix performance issues

A) When a QEdidParser gets built, the code populates a QMap by parsing
the system's pnp ids file. With > 2000 entries in the file, that's 2000+
allocations for the QMap, plus 4000+ allocations for key/value
(QStrings), plus a bunch of temporaries created for processing.

What's more, on XCB and EGLFS, a QEdidParser gets built *per screen*;
the map is not shared and gets rebuilt for each screen.

It's however completely unnecessary to keep this map in memory.  The
lookup is required only once per monitor change event, which, apart from
application startup, is an extremely rare event.

When such an event happens, and we have to lookup a vendor id, we can
just process the pnp database "on the fly", therefore removing the
constant memory usage from the application run.

In order to also avoid an allocation spike, just don't build a map at
all: simply process the file, using no memory allocations, one line at a
time while searching for the vendor id. I was unable to find
information about the file format, so I am not sure if it's guaranteed
that it's kept sorted by id (this could allow for a faster fail path).

B) In case of a cache miss, a hardcoded vendor table is used to try and
identify the manufacturer.

Look up in the hardcoded table using binary search, rather than a linear
scan, and don't allocate memory for each element processed in the list
(!). The size of the list (2000+) is big enough to completely justify
binary search.

C) Drive-by, remove a TOCTOU bug when opening the system file.

D) Drive-by, fix the includes in the header to IWYU.

Change-Id: I57c7cbd09a145c6efe3023c227ed36b24bed96f9
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

2 files changed, 58 insertions, 34 deletions

diff --git a/src/gui/util/qedidparser.cpp b/src/gui/util/qedidparser.cpp
index 553ca20807..db5ccf1772 100644
--- a/src/gui/util/qedidparser.cpp
+++ b/src/gui/util/qedidparser.cpp
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2017 Pier Luigi Fiorini <pierluigi.fiorini@gmail.com>
+** Copyright (C) 2021 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com, author Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtGui module of the Qt Toolkit.
@@ -38,6 +39,7 @@
 ****************************************************************************/
 
 #include <QtCore/QFile>
+#include <QtCore/QByteArrayView>
 
 #include "qedidparser_p.h"
 #include "qedidvendortable_p.h"
@@ -58,31 +60,46 @@
 
 QT_BEGIN_NAMESPACE
 
-QEdidParser::QEdidParser()
+static QString lookupVendorIdInSystemDatabase(QByteArrayView id)
 {
-    // Cache vendors list from pnp.ids
+    QString result;
+
     const QString fileName = QLatin1String("/usr/share/hwdata/pnp.ids");
-    if (QFile::exists(fileName)) {
-        QFile file(fileName);
-
-        if (file.open(QFile::ReadOnly)) {
-            while (!file.atEnd()) {
-                QString line = QString::fromUtf8(file.readLine()).trimmed();
-
-                if (line.startsWith(QLatin1Char('#')))
-                    continue;
-
-                QStringList parts = line.split(QLatin1Char('\t'));
-                if (parts.count() > 1) {
-                    QString pnpId = parts.at(0);
-                    parts.removeFirst();
-                    m_vendorCache[pnpId] = parts.join(QLatin1Char(' '));
-                }
-            }
+    QFile file(fileName);
+    if (!file.open(QFile::ReadOnly))
+        return result;
+
+    // On Ubuntu 20.04 the longest line in the file is 85 bytes, so this
+    // leaves plenty of room...
+    constexpr int MaxLineSize = 512;
+    char buf[MaxLineSize];
 
-            file.close();
+    while (!file.atEnd()) {
+        auto read = file.readLine(buf, MaxLineSize);
+        if (read < 0 || read == MaxLineSize) // read error
+            break;
+
+        QByteArrayView line(buf, read - 1); // -1 to remove the trailing newline
+        if (line.isEmpty())
+            continue;
+
+        if (line.startsWith('#'))
+            continue;
+
+        auto tabPosition = line.indexOf('\t');
+        if (tabPosition <= 0) // no vendor id
+            continue;
+        if (tabPosition + 1 == line.size()) // no vendor name
+            continue;
+
+        if (line.first(tabPosition) == id) {
+            auto vendor = line.sliced(tabPosition + 1);
+            result = QString::fromUtf8(vendor.data(), vendor.size());
+            break;
         }
     }
+
+    return result;
 }
 
 bool QEdidParser::parse(const QByteArray &blob)
@@ -105,7 +122,6 @@ bool QEdidParser::parse(const QByteArray &blob)
     pnpId[0] = 'A' + ((data[EDID_OFFSET_PNP_ID] & 0x7c) / 4) - 1;
     pnpId[1] = 'A' + ((data[EDID_OFFSET_PNP_ID] & 0x3) * 8) + ((data[EDID_OFFSET_PNP_ID + 1] & 0xe0) / 32) - 1;
     pnpId[2] = 'A' + (data[EDID_OFFSET_PNP_ID + 1] & 0x1f) - 1;
-    QString pnpIdString = QString::fromLatin1(pnpId, 3);
 
     // Clear manufacturer
     manufacturer = QString();
@@ -137,20 +153,29 @@ bool QEdidParser::parse(const QByteArray &blob)
     }
 
     // Try to use cache first because it is potentially more updated
-    manufacturer = m_vendorCache.value(pnpIdString);
+    manufacturer = lookupVendorIdInSystemDatabase(pnpId);
+
     if (manufacturer.isEmpty()) {
         // Find the manufacturer from the vendor lookup table
-        for (const auto &vendor : q_edidVendorTable) {
-            if (strncmp(vendor.id, pnpId, 3) == 0) {
-                manufacturer = QString::fromUtf8(vendor.name);
-                break;
-            }
-        }
+        const auto compareVendorId = [](const VendorTable &vendor, const char *str)
+        {
+            return strncmp(vendor.id, str, 3) < 0;
+        };
+
+        const auto b = std::begin(q_edidVendorTable);
+        const auto e = std::end(q_edidVendorTable);
+        auto it = std::lower_bound(b,
+                                   e,
+                                   pnpId,
+                                   compareVendorId);
+
+        if (it != e && strncmp(it->id, pnpId, 3) == 0)
+            manufacturer = QString::fromUtf8(it->name);
     }
 
     // If we don't know the manufacturer, fallback to PNP ID
     if (manufacturer.isEmpty())
-        manufacturer = pnpIdString;
+        manufacturer = QString::fromUtf8(pnpId, std::size(pnpId));
 
     // Physical size
     physicalSize = QSizeF(data[EDID_PHYSICAL_WIDTH], data[EDID_OFFSET_PHYSICAL_HEIGHT]) * 10;
diff --git a/src/gui/util/qedidparser_p.h b/src/gui/util/qedidparser_p.h
index 14c736f4cc..f18c2f1ad1 100644
--- a/src/gui/util/qedidparser_p.h
+++ b/src/gui/util/qedidparser_p.h
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2017 Pier Luigi Fiorini <pierluigi.fiorini@gmail.com>
+** Copyright (C) 2021 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com, author Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtGui module of the Qt Toolkit.
@@ -56,16 +57,16 @@
 //
 
 #include <QtGui/qtguiglobal.h>
+#include <QtCore/qlist.h>
+#include <QtCore/qpoint.h>
+#include <QtCore/qsize.h>
 #include <QtCore/qstring.h>
-#include <QtCore/qmap.h>
 
 QT_BEGIN_NAMESPACE
 
 class Q_GUI_EXPORT QEdidParser
 {
 public:
-    QEdidParser();
-
     bool parse(const QByteArray &blob);
 
     QString identifier;
@@ -83,8 +84,6 @@ public:
     bool useTables;
 
 private:
-    QMap<QString, QString> m_vendorCache;
-
     QString parseEdidString(const quint8 *data);
 };