Clean up the QFontEngine glyphcaching code to not crash and be more tidy

Reviewed-by: Eskil

2 files changed, 28 insertions, 114 deletions

diff --git a/src/gui/text/qfontengine.cpp b/src/gui/text/qfontengine.cpp
index 0eadf0469b..d364025449 100644
--- a/src/gui/text/qfontengine.cpp
+++ b/src/gui/text/qfontengine.cpp
@@ -185,22 +185,11 @@ QFontEngine::QFontEngine()
 
 QFontEngine::~QFontEngine()
 {
-    for (GlyphPointerHash::const_iterator it = m_glyphPointerHash.constBegin(),
-            end = m_glyphPointerHash.constEnd(); it != end; ++it) {
-        for (QList<QFontEngineGlyphCache*>::const_iterator it2 = it.value().constBegin(),
-                end2 = it.value().constEnd(); it2 != end2; ++it2) {
-            delete *it2;
-        }
-    }
-    m_glyphPointerHash.clear();
-    for (GlyphIntHash::const_iterator it = m_glyphIntHash.constBegin(),
-            end = m_glyphIntHash.constEnd(); it != end; ++it) {
-        for (QList<QFontEngineGlyphCache*>::const_iterator it2 = it.value().constBegin(),
-                end2 = it.value().constEnd(); it2 != end2; ++it2) {
-            delete *it2;
-        }
+    for (QLinkedList<GlyphCacheEntry>::const_iterator it = m_glyphCaches.constBegin(),
+            end = m_glyphCaches.constEnd(); it != end; ++it) {
+        delete it->cache;
     }
-    m_glyphIntHash.clear();
+    m_glyphCaches.clear();
     qHBFreeFace(hbFace);
 }
 
@@ -713,105 +702,30 @@ QByteArray QFontEngine::getSfntTable(uint tag) const
     return table;
 }
 
-void QFontEngine::expireGlyphCache()
-{
-    if (m_glyphCacheQueue.count() > 10) { // hold only 10 caches in memory.
-        QFontEngineGlyphCache *old = m_glyphCacheQueue.takeFirst();
-        // remove the value from either of our hashes
-        for (GlyphPointerHash::iterator i = m_glyphPointerHash.begin(); i != m_glyphPointerHash.end(); ++i) {
-            QList<QFontEngineGlyphCache *> list = i.value();
-            if (list.removeAll(old)) {
-                if (list.isEmpty())
-                    m_glyphPointerHash.remove(i.key());
-                else
-                    m_glyphPointerHash.insert(i.key(), list);
-                break;
-            }
-        }
-        for (GlyphIntHash::iterator i = m_glyphIntHash.begin(); i != m_glyphIntHash.end(); ++i) {
-            QList<QFontEngineGlyphCache *> list = i.value();
-            if (list.removeAll(old)) {
-                if (list.isEmpty())
-                    m_glyphIntHash.remove(i.key());
-                else
-                    m_glyphIntHash.insert(i.key(), list);
-                break;
-            }
-        }
-        delete old;
-    }
-}
-
-void QFontEngine::setGlyphCache(void *key, QFontEngineGlyphCache *data, QFontEngineGlyphCache::Type type)
+void QFontEngine::setGlyphCache(void *key, QFontEngineGlyphCache *data)
 {
     Q_ASSERT(data);
-    QList<QFontEngineGlyphCache*> items = m_glyphPointerHash.value(key);
-
-    for (QList<QFontEngineGlyphCache*>::iterator it = items.begin(), end = items.end(); it != end; ++it) {
-        QFontEngineGlyphCache *c = *it;
-        if (qtransform_equals_no_translate(c->m_transform, data->m_transform)
-            && c->cacheType() == type) {
-            if (c == data)
-                return;
-            items.removeAll(c);
-            delete c;
-            break;
-        }
-    }
-    items.append(data);
-    m_glyphPointerHash.insert(key, items);
 
-    m_glyphCacheQueue.append(data);
-    expireGlyphCache();
-}
+    GlyphCacheEntry entry = { key, data };
+    if (m_glyphCaches.contains(entry))
+        return;
 
-void QFontEngine::setGlyphCache(QFontEngineGlyphCache::Type key, QFontEngineGlyphCache *data)
-{
-    Q_ASSERT(data);
-    QList<QFontEngineGlyphCache*> items = m_glyphIntHash.value(key);
-
-    for (QList<QFontEngineGlyphCache*>::iterator it = items.begin(), end = items.end(); it != end; ++it) {
-        QFontEngineGlyphCache *c = *it;
-        if (qtransform_equals_no_translate(c->m_transform, data->m_transform)) {
-            if (c == data)
-                return;
-            items.removeAll(c);
-            delete c;
-            break;
-        }
-    }
-    items.append(data);
-    m_glyphIntHash.insert(key, items);
+    // Limit the glyph caches to 4. This covers all 90 degree rotations and limits
+    // memory use when there is continous or random rotation
+    if (m_glyphCaches.size() == 4)
+        delete m_glyphCaches.takeLast().cache;
 
-    m_glyphCacheQueue.append(data);
-    expireGlyphCache();
-}
+    m_glyphCaches.push_front(entry);
 
-QFontEngineGlyphCache *QFontEngine::glyphCache(void *key, const QTransform &transform, QFontEngineGlyphCache::Type type) const
-{
-    QList<QFontEngineGlyphCache*> items = m_glyphPointerHash.value(key);
-
-    for (QList<QFontEngineGlyphCache*>::iterator it = items.begin(), end = items.end(); it != end; ++it) {
-        QFontEngineGlyphCache *c = *it;
-        if (qtransform_equals_no_translate(c->m_transform, transform)
-            && type == c->cacheType()) {
-            m_glyphCacheQueue.removeAll(c); // last used, move it up
-            m_glyphCacheQueue.append(c);
-            return c;
-        }
-    }
-    return 0;
 }
 
-QFontEngineGlyphCache *QFontEngine::glyphCache(QFontEngineGlyphCache::Type key, const QTransform &transform) const
+QFontEngineGlyphCache *QFontEngine::glyphCache(void *key, QFontEngineGlyphCache::Type type, const QTransform &transform) const
 {
-    QList<QFontEngineGlyphCache*> items = m_glyphIntHash.value(key);
-
-    for (QList<QFontEngineGlyphCache*>::iterator it = items.begin(), end = items.end(); it != end; ++it) {
-        QFontEngineGlyphCache *c = *it;
-        if (qtransform_equals_no_translate(c->m_transform, transform)) {
-            m_glyphCacheQueue.removeAll(c); // last used, move it up
-            m_glyphCacheQueue.append(c);
+    for (QLinkedList<GlyphCacheEntry>::const_iterator it = m_glyphCaches.constBegin(), end = m_glyphCaches.constEnd(); it != end; ++it) {
+        QFontEngineGlyphCache *c = it->cache;
+        if (key == it->context
+            && type == c->cacheType()
+            && qtransform_equals_no_translate(c->m_transform, transform)) {
             return c;
         }
     }
diff --git a/src/gui/text/qfontengine_p.h b/src/gui/text/qfontengine_p.h
index 62bff8536b..a9883b4af7 100644
--- a/src/gui/text/qfontengine_p.h
+++ b/src/gui/text/qfontengine_p.h
@@ -56,6 +56,7 @@
 #include "QtCore/qglobal.h"
 #include "QtCore/qatomic.h"
 #include <QtCore/qvarlengtharray.h>
+#include <QtCore/QLinkedList>
 #include "private/qtextengine_p.h"
 #include "private/qfont_p.h"
 
@@ -218,10 +219,8 @@ public:
 
     virtual HB_Error getPointInOutline(HB_Glyph glyph, int flags, hb_uint32 point, HB_Fixed *xpos, HB_Fixed *ypos, hb_uint32 *nPoints);
 
-    void setGlyphCache(void *key, QFontEngineGlyphCache *data, QFontEngineGlyphCache::Type type);
-    void setGlyphCache(QFontEngineGlyphCache::Type key, QFontEngineGlyphCache *data);
-    QFontEngineGlyphCache *glyphCache(void *key, const QTransform &transform, QFontEngineGlyphCache::Type type) const;
-    QFontEngineGlyphCache *glyphCache(QFontEngineGlyphCache::Type key, const QTransform &transform) const;
+    void setGlyphCache(void *key, QFontEngineGlyphCache *data);
+    QFontEngineGlyphCache *glyphCache(void *key, QFontEngineGlyphCache::Type type, const QTransform &transform) const;
 
     static const uchar *getCMap(const uchar *table, uint tableSize, bool *isSymbolFont, int *cmapSize);
     static quint32 getTrueTypeGlyphIndex(const uchar *cmap, uint unicode);
@@ -254,12 +253,13 @@ protected:
     static const QVector<QRgb> &grayPalette();
 
 private:
-    /// remove old entries from the glyph cache. Helper method for the setGlyphCache ones.
-    void expireGlyphCache();
+    struct GlyphCacheEntry {
+        void *context;
+        QFontEngineGlyphCache *cache;
+        bool operator==(const GlyphCacheEntry &other) { return context == other.context && cache == other.cache; }
+    };
 
-    GlyphPointerHash m_glyphPointerHash;
-    GlyphIntHash m_glyphIntHash;
-    mutable QList<QFontEngineGlyphCache*> m_glyphCacheQueue;
+    mutable QLinkedList<GlyphCacheEntry> m_glyphCaches;
 };
 
 inline bool operator ==(const QFontEngine::FaceId &f1, const QFontEngine::FaceId &f2)