xcb: fix bitmap cursor loading performance regression

... introduced by 422838685c31d9b57133a8711bfd5db92095d96d.
Instead of completely droping caching for bitmap cursors we can do
the same what is done in libXcursor - have a fixed size cache, with
oldest entries eventually being replaced with new bitmaps. This
fixes the original issue, where the hash was growing indefinitely
until running out of file descriptors and won't have the performance
penalty as in 422838685c31d9b57133a8711bfd5db92095d96d.

Task-number: QTBUG-66897
Change-Id: I14f80b46f97fd0e2c920e17a31ffbc0441cd9d22
Reviewed-by: Uli Schlachter <psychon@znc.in>
Reviewed-by: Robin Burchell <robin.burchell@crimson.no>
Reviewed-by: Laszlo Agocs <laszlo.agocs@qt.io>

4 files changed, 38 insertions, 45 deletions

diff --git a/src/plugins/platforms/xcb/qxcbcursor.cpp b/src/plugins/platforms/xcb/qxcbcursor.cpp
index 34b7d0d236..70138abede 100644
--- a/src/plugins/platforms/xcb/qxcbcursor.cpp
+++ b/src/plugins/platforms/xcb/qxcbcursor.cpp
@@ -301,6 +301,9 @@ QXcbCursorCacheKey::QXcbCursorCacheKey(const QCursor &c)
 QXcbCursor::QXcbCursor(QXcbConnection *conn, QXcbScreen *screen)
     : QXcbObject(conn), m_screen(screen), m_gtkCursorThemeInitialized(false)
 {
+    // see NUM_BITMAPS in libXcursor/src/xcursorint.h
+    m_bitmapCache.setMaxCost(8);
+
     if (cursorCount++)
         return;
 
@@ -351,37 +354,38 @@ QXcbCursor::~QXcbCursor()
 }
 
 #ifndef QT_NO_CURSOR
-void QXcbCursor::changeCursor(QCursor *cursor, QWindow *widget)
+void QXcbCursor::changeCursor(QCursor *cursor, QWindow *window)
 {
-    QXcbWindow *w = 0;
-    if (widget && widget->handle())
-        w = static_cast<QXcbWindow *>(widget->handle());
-    else
-        // No X11 cursor control when there is no widget under the cursor
+    if (!window || !window->handle())
         return;
 
     xcb_cursor_t c = XCB_CURSOR_NONE;
-    bool isBitmapCursor = false;
-
     if (cursor) {
+        const QXcbCursorCacheKey key(*cursor);
         const Qt::CursorShape shape = cursor->shape();
-        isBitmapCursor = shape == Qt::BitmapCursor;
 
-        if (!isBitmapCursor) {
-            const QXcbCursorCacheKey key(*cursor);
-            CursorHash::iterator it = m_cursorHash.find(key);
-            if (it == m_cursorHash.end()) {
-                it = m_cursorHash.insert(key, createFontCursor(shape));
+        if (shape == Qt::BitmapCursor) {
+            auto *bitmap = m_bitmapCache.object(key);
+            if (bitmap) {
+                c = bitmap->cursor;
+            } else {
+                c = createBitmapCursor(cursor);
+                m_bitmapCache.insert(key, new CachedCursor(xcb_connection(), c));
             }
-            c = it.value();
         } else {
-            // Do not cache bitmap cursors, as otherwise they have unclear
-            // lifetime (we effectively leak xcb_cursor_t).
-            c = createBitmapCursor(cursor);
+            auto it = m_cursorHash.find(key);
+            if (it == m_cursorHash.end()) {
+                c = createFontCursor(shape);
+                m_cursorHash.insert(key, c);
+            } else {
+                c = it.value();
+            }
         }
     }
 
-    w->setCursor(c, isBitmapCursor);
+    auto *w = static_cast<QXcbWindow *>(window->handle());
+    xcb_change_window_attributes(xcb_connection(), w->xcb_window(), XCB_CW_CURSOR, &c);
+    xcb_flush(xcb_connection());
 }
 
 static int cursorIdForShape(int cshape)
diff --git a/src/plugins/platforms/xcb/qxcbcursor.h b/src/plugins/platforms/xcb/qxcbcursor.h
index e3f88518fe..5bc806381c 100644
--- a/src/plugins/platforms/xcb/qxcbcursor.h
+++ b/src/plugins/platforms/xcb/qxcbcursor.h
@@ -43,6 +43,8 @@
 #include <qpa/qplatformcursor.h>
 #include "qxcbscreen.h"
 
+#include <QtCore/QCache>
+
 QT_BEGIN_NAMESPACE
 
 #ifndef QT_NO_CURSOR
@@ -76,7 +78,7 @@ public:
     QXcbCursor(QXcbConnection *conn, QXcbScreen *screen);
     ~QXcbCursor();
 #ifndef QT_NO_CURSOR
-    void changeCursor(QCursor *cursor, QWindow *widget) override;
+    void changeCursor(QCursor *cursor, QWindow *window) override;
 #endif
     QPoint pos() const override;
     void setPos(const QPoint &pos) override;
@@ -89,9 +91,20 @@ public:
 #endif
 
 private:
+
 #ifndef QT_NO_CURSOR
     typedef QHash<QXcbCursorCacheKey, xcb_cursor_t> CursorHash;
 
+    struct CachedCursor
+    {
+        explicit CachedCursor(xcb_connection_t *conn, xcb_cursor_t c)
+            : cursor(c), connection(conn) {}
+        ~CachedCursor() { xcb_free_cursor(connection, cursor); }
+        xcb_cursor_t cursor;
+        xcb_connection_t *connection;
+    };
+    typedef QCache<QXcbCursorCacheKey, CachedCursor> BitmapCursorCache;
+
     xcb_cursor_t createFontCursor(int cshape);
     xcb_cursor_t createBitmapCursor(QCursor *cursor);
     xcb_cursor_t createNonStandardCursor(int cshape);
@@ -100,6 +113,7 @@ private:
     QXcbScreen *m_screen;
 #ifndef QT_NO_CURSOR
     CursorHash m_cursorHash;
+    BitmapCursorCache m_bitmapCache;
 #endif
 #if QT_CONFIG(xcb_xlib) && QT_CONFIG(library)
     static void cursorThemePropertyChanged(QXcbVirtualDesktop *screen,
diff --git a/src/plugins/platforms/xcb/qxcbwindow.cpp b/src/plugins/platforms/xcb/qxcbwindow.cpp
index fed096f311..f87da28ee0 100644
--- a/src/plugins/platforms/xcb/qxcbwindow.cpp
+++ b/src/plugins/platforms/xcb/qxcbwindow.cpp
@@ -562,10 +562,6 @@ void QXcbWindow::create()
 
 QXcbWindow::~QXcbWindow()
 {
-    if (m_currentBitmapCursor != XCB_CURSOR_NONE) {
-        xcb_free_cursor(xcb_connection(), m_currentBitmapCursor);
-    }
-
     destroy();
 }
 
@@ -2591,24 +2587,6 @@ bool QXcbWindow::setMouseGrabEnabled(bool grab)
     return result;
 }
 
-void QXcbWindow::setCursor(xcb_cursor_t cursor, bool isBitmapCursor)
-{
-    xcb_connection_t *conn = xcb_connection();
-
-    xcb_change_window_attributes(conn, m_window, XCB_CW_CURSOR, &cursor);
-    xcb_flush(conn);
-
-    if (m_currentBitmapCursor != XCB_CURSOR_NONE) {
-        xcb_free_cursor(conn, m_currentBitmapCursor);
-    }
-
-    if (isBitmapCursor) {
-        m_currentBitmapCursor = cursor;
-    } else {
-        m_currentBitmapCursor = XCB_CURSOR_NONE;
-    }
-}
-
 void QXcbWindow::windowEvent(QEvent *event)
 {
     switch (event->type()) {
diff --git a/src/plugins/platforms/xcb/qxcbwindow.h b/src/plugins/platforms/xcb/qxcbwindow.h
index 957c4e9cbd..cb8af9e666 100644
--- a/src/plugins/platforms/xcb/qxcbwindow.h
+++ b/src/plugins/platforms/xcb/qxcbwindow.h
@@ -103,8 +103,6 @@ public:
     bool setKeyboardGrabEnabled(bool grab) override;
     bool setMouseGrabEnabled(bool grab) override;
 
-    void setCursor(xcb_cursor_t cursor, bool isBitmapCursor);
-
     QSurfaceFormat format() const override;
 
     void windowEvent(QEvent *event) override;
@@ -285,7 +283,6 @@ protected:
     SyncState m_syncState = NoSyncNeeded;
 
     QXcbSyncWindowRequest *m_pendingSyncRequest = nullptr;
-    xcb_cursor_t m_currentBitmapCursor = XCB_CURSOR_NONE;
 };
 
 class QXcbForeignWindow : public QXcbWindow