Trigger the garbage collector when allocating InternalClass objects

As we check the icAllocator's slots on shouldRunGC() we should also
check shouldRunGC() when adding slots. Otherwise we might never run the
GC when only allocating InternalClasses. In addition, account for the
"unmanaged" size of the PropertyAttributes that are part of the
InternalClass objects. Those can be large.

In cases where an excessive number of large InternalClass objects is
created the garbage collector is now invoked frequently, which costs a
significant number of CPU cycles, but prevents the memory usage from
growing indefinitely.

Task-number: QTBUG-58559
Change-Id: Icf102cb6100f6dba212b8bffe1c178897880eda0
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

5 files changed, 152 insertions, 114 deletions

diff --git a/src/qml/jsruntime/qv4internalclass.cpp b/src/qml/jsruntime/qv4internalclass.cpp
index 9906e2b1a0..8e2657f4a5 100644
--- a/src/qml/jsruntime/qv4internalclass.cpp
+++ b/src/qml/jsruntime/qv4internalclass.cpp
@@ -44,6 +44,7 @@
 #include "qv4object_p.h"
 #include "qv4identifiertable_p.h"
 #include "qv4value_p.h"
+#include "qv4mm_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -204,7 +205,62 @@ void SharedInternalClassDataPrivate<PropertyKey>::mark(MarkStack *s)
         data->mark(s);
 }
 
+SharedInternalClassDataPrivate<PropertyAttributes>::SharedInternalClassDataPrivate(
+        const SharedInternalClassDataPrivate<PropertyAttributes> &other, uint pos,
+        PropertyAttributes value)
+    : refcount(1),
+      m_alloc(pos + 8),
+      m_size(pos + 1),
+      m_engine(other.m_engine)
+{
+    m_engine->memoryManager->changeUnmanagedHeapSizeUsage(m_alloc * sizeof(PropertyAttributes));
+    data = new PropertyAttributes[m_alloc];
+    if (other.data)
+        memcpy(data, other.data, (m_size - 1) * sizeof(PropertyAttributes));
+    data[pos] = value;
+}
+
+SharedInternalClassDataPrivate<PropertyAttributes>::SharedInternalClassDataPrivate(
+        const SharedInternalClassDataPrivate<PropertyAttributes> &other)
+    : refcount(1),
+      m_alloc(other.m_alloc),
+      m_size(other.m_size),
+      m_engine(other.m_engine)
+{
+    if (m_alloc) {
+        m_engine->memoryManager->changeUnmanagedHeapSizeUsage(m_alloc * sizeof(PropertyAttributes));
+        data = new PropertyAttributes[m_alloc];
+        memcpy(data, other.data, m_size*sizeof(PropertyAttributes));
+    } else {
+        data = nullptr;
+    }
+}
 
+SharedInternalClassDataPrivate<PropertyAttributes>::~SharedInternalClassDataPrivate()
+{
+    m_engine->memoryManager->changeUnmanagedHeapSizeUsage(
+            -qptrdiff(m_alloc * sizeof(PropertyAttributes)));
+    delete [] data;
+}
+
+void SharedInternalClassDataPrivate<PropertyAttributes>::grow() {
+    if (!m_alloc) {
+        m_alloc = 4;
+        m_engine->memoryManager->changeUnmanagedHeapSizeUsage(
+                2 * m_alloc * sizeof(PropertyAttributes));
+    } else {
+        m_engine->memoryManager->changeUnmanagedHeapSizeUsage(
+                m_alloc * sizeof(PropertyAttributes));
+    }
+
+    auto *n = new PropertyAttributes[m_alloc * 2];
+    if (data) {
+        memcpy(n, data, m_alloc*sizeof(PropertyAttributes));
+        delete [] data;
+    }
+    data = n;
+    m_alloc *= 2;
+}
 
 namespace Heap {
 
diff --git a/src/qml/jsruntime/qv4internalclass_p.h b/src/qml/jsruntime/qv4internalclass_p.h
index 681cbda5f9..c689637721 100644
--- a/src/qml/jsruntime/qv4internalclass_p.h
+++ b/src/qml/jsruntime/qv4internalclass_p.h
@@ -149,55 +149,31 @@ inline PropertyHash::Entry *PropertyHash::lookup(PropertyKey identifier) const
     }
 }
 
-template<typename T>
-struct SharedInternalClassDataPrivate {
-    SharedInternalClassDataPrivate(ExecutionEngine *)
+template<class T>
+struct SharedInternalClassDataPrivate {};
+
+template<>
+struct SharedInternalClassDataPrivate<PropertyAttributes> {
+    SharedInternalClassDataPrivate(ExecutionEngine *engine)
         : refcount(1),
           m_alloc(0),
           m_size(0),
-          data(nullptr)
+          data(nullptr),
+          m_engine(engine)
     { }
-    SharedInternalClassDataPrivate(const SharedInternalClassDataPrivate &other)
-        : refcount(1),
-          m_alloc(other.m_alloc),
-          m_size(other.m_size)
-    {
-        if (m_alloc) {
-            data = new T[m_alloc];
-            memcpy(data, other.data, m_size*sizeof(T));
-        }
-    }
-    SharedInternalClassDataPrivate(const SharedInternalClassDataPrivate &other, uint pos, T value)
-        : refcount(1),
-          m_alloc(pos + 8),
-          m_size(pos + 1)
-    {
-        data = new T[m_alloc];
-        if (other.data)
-            memcpy(data, other.data, (m_size - 1)*sizeof(T));
-        data[pos] = value;
-    }
-    ~SharedInternalClassDataPrivate() { delete [] data; }
-
+    SharedInternalClassDataPrivate(const SharedInternalClassDataPrivate<PropertyAttributes> &other);
+    SharedInternalClassDataPrivate(const SharedInternalClassDataPrivate<PropertyAttributes> &other,
+                                   uint pos, PropertyAttributes value);
+    ~SharedInternalClassDataPrivate();
 
-    void grow() {
-        if (!m_alloc)
-            m_alloc = 4;
-        T *n = new T[m_alloc * 2];
-        if (data) {
-            memcpy(n, data, m_alloc*sizeof(T));
-            delete [] data;
-        }
-        data = n;
-        m_alloc *= 2;
-    }
+    void grow();
 
     uint alloc() const { return m_alloc; }
     uint size() const { return m_size; }
     void setSize(uint s) { m_size = s; }
 
-    T at(uint i) { Q_ASSERT(data && i < m_alloc); return data[i]; }
-    void set(uint i, T t) { Q_ASSERT(data && i < m_alloc); data[i] = t; }
+    PropertyAttributes at(uint i) { Q_ASSERT(data && i < m_alloc); return data[i]; }
+    void set(uint i, PropertyAttributes t) { Q_ASSERT(data && i < m_alloc); data[i] = t; }
 
     void mark(MarkStack *) {}
 
@@ -205,7 +181,8 @@ struct SharedInternalClassDataPrivate {
 private:
     uint m_alloc;
     uint m_size;
-    T *data;
+    PropertyAttributes *data;
+    ExecutionEngine *m_engine;
 };
 
 template<>
diff --git a/src/qml/memory/qv4mm.cpp b/src/qml/memory/qv4mm.cpp
index 203f1f424f..cac68bdcaf 100644
--- a/src/qml/memory/qv4mm.cpp
+++ b/src/qml/memory/qv4mm.cpp
@@ -93,8 +93,6 @@
 #include <pthread_np.h>
 #endif
 
-#define MIN_UNMANAGED_HEAPSIZE_GC_LIMIT std::size_t(128 * 1024)
-
 Q_LOGGING_CATEGORY(lcGcStats, "qt.qml.gc.statistics")
 Q_DECLARE_LOGGING_CATEGORY(lcGcStats)
 Q_LOGGING_CATEGORY(lcGcAllocatorStats, "qt.qml.gc.allocatorStats")
@@ -759,7 +757,7 @@ MemoryManager::MemoryManager(ExecutionEngine *engine)
     , hugeItemAllocator(chunkAllocator, engine)
     , m_persistentValues(new PersistentValueStorage(engine))
     , m_weakValues(new PersistentValueStorage(engine))
-    , unmanagedHeapSizeGCLimit(MIN_UNMANAGED_HEAPSIZE_GC_LIMIT)
+    , unmanagedHeapSizeGCLimit(MinUnmanagedHeapSizeGCLimit)
     , aggressiveGC(!qEnvironmentVariableIsEmpty("QV4_MM_AGGRESSIVE_GC"))
     , gcStats(lcGcStats().isDebugEnabled())
     , gcCollectorStats(lcGcAllocatorStats().isDebugEnabled())
@@ -779,35 +777,9 @@ Heap::Base *MemoryManager::allocString(std::size_t unmanagedSize)
     lastAllocRequestedSlots = stringSize >> Chunk::SlotSizeShift;
     ++allocationCount;
 #endif
-
-    bool didGCRun = false;
-    if (aggressiveGC) {
-        runGC();
-        didGCRun = true;
-    }
-
     unmanagedHeapSize += unmanagedSize;
-    if (unmanagedHeapSize > unmanagedHeapSizeGCLimit) {
-        if (!didGCRun)
-            runGC();
-
-        if (3*unmanagedHeapSizeGCLimit <= 4*unmanagedHeapSize)
-            // more than 75% full, raise limit
-            unmanagedHeapSizeGCLimit = std::max(unmanagedHeapSizeGCLimit, unmanagedHeapSize) * 2;
-        else if (unmanagedHeapSize * 4 <= unmanagedHeapSizeGCLimit)
-            // less than 25% full, lower limit
-            unmanagedHeapSizeGCLimit = qMax(MIN_UNMANAGED_HEAPSIZE_GC_LIMIT, unmanagedHeapSizeGCLimit/2);
-        didGCRun = true;
-    }
-
-    HeapItem *m = blockAllocator.allocate(stringSize);
-    if (!m) {
-        if (!didGCRun && shouldRunGC())
-            runGC();
-        m = blockAllocator.allocate(stringSize, true);
-    }
 
-//    qDebug() << "allocated string" << m;
+    HeapItem *m = allocate(&blockAllocator, stringSize);
     memset(m, 0, stringSize);
     return *m;
 }
@@ -819,32 +791,11 @@ Heap::Base *MemoryManager::allocData(std::size_t size)
     ++allocationCount;
 #endif
 
-    bool didRunGC = false;
-    if (aggressiveGC) {
-        runGC();
-        didRunGC = true;
-    }
-
     Q_ASSERT(size >= Chunk::SlotSize);
     Q_ASSERT(size % Chunk::SlotSize == 0);
 
-//    qDebug() << "unmanagedHeapSize:" << unmanagedHeapSize << "limit:" << unmanagedHeapSizeGCLimit << "unmanagedSize:" << unmanagedSize;
-
-    if (size > Chunk::DataSize) {
-        HeapItem *h = hugeItemAllocator.allocate(size);
-//        qDebug() << "allocating huge item" << h;
-        return *h;
-    }
-
-    HeapItem *m = blockAllocator.allocate(size);
-    if (!m) {
-        if (!didRunGC && shouldRunGC())
-            runGC();
-        m = blockAllocator.allocate(size, true);
-    }
-
+    HeapItem *m = allocate(&blockAllocator, size);
     memset(m, 0, size);
-//    qDebug() << "allocating data" << m;
     return *m;
 }
 
diff --git a/src/qml/memory/qv4mm_p.h b/src/qml/memory/qv4mm_p.h
index bbbbb1aef6..6dfdd81cb2 100644
--- a/src/qml/memory/qv4mm_p.h
+++ b/src/qml/memory/qv4mm_p.h
@@ -264,13 +264,13 @@ public:
     size_t getLargeItemsMem() const;
 
     // called when a JS object grows itself. Specifically: Heap::String::append
+    // and InternalClassDataPrivate<PropertyAttributes>.
     void changeUnmanagedHeapSizeUsage(qptrdiff delta) { unmanagedHeapSize += delta; }
 
     template<typename ManagedType>
     typename ManagedType::Data *allocIC()
     {
-        size_t size = align(sizeof(typename ManagedType::Data));
-        Heap::Base *b = *icAllocator.allocate(size, true);
+        Heap::Base *b = *allocate(&icAllocator, align(sizeof(typename ManagedType::Data)));
         return static_cast<typename ManagedType::Data *>(b);
     }
 
@@ -284,12 +284,52 @@ protected:
     Heap::Object *allocObjectWithMemberData(const QV4::VTable *vtable, uint nMembers);
 
 private:
+    enum {
+        MinUnmanagedHeapSizeGCLimit = 128 * 1024
+    };
+
     void collectFromJSStack(MarkStack *markStack) const;
     void mark();
     void sweep(bool lastSweep = false, ClassDestroyStatsCallback classCountPtr = nullptr);
     bool shouldRunGC() const;
     void collectRoots(MarkStack *markStack);
 
+    HeapItem *allocate(BlockAllocator *allocator, std::size_t size)
+    {
+        bool didGCRun = false;
+        if (aggressiveGC) {
+            runGC();
+            didGCRun = true;
+        }
+
+        if (unmanagedHeapSize > unmanagedHeapSizeGCLimit) {
+            if (!didGCRun)
+                runGC();
+
+            if (3*unmanagedHeapSizeGCLimit <= 4 * unmanagedHeapSize) {
+                // more than 75% full, raise limit
+                unmanagedHeapSizeGCLimit = std::max(unmanagedHeapSizeGCLimit,
+                                                    unmanagedHeapSize) * 2;
+            } else if (unmanagedHeapSize * 4 <= unmanagedHeapSizeGCLimit) {
+                // less than 25% full, lower limit
+                unmanagedHeapSizeGCLimit = qMax(std::size_t(MinUnmanagedHeapSizeGCLimit),
+                                                unmanagedHeapSizeGCLimit/2);
+            }
+            didGCRun = true;
+        }
+
+        if (size > Chunk::DataSize)
+            return hugeItemAllocator.allocate(size);
+
+        if (HeapItem *m = allocator->allocate(size))
+            return m;
+
+        if (!didGCRun && shouldRunGC())
+            runGC();
+
+        return allocator->allocate(size, true);
+    }
+
 public:
     QV4::ExecutionEngine *engine;
     ChunkAllocator *chunkAllocator;
diff --git a/tests/auto/qml/qv4mm/tst_qv4mm.cpp b/tests/auto/qml/qv4mm/tst_qv4mm.cpp
index b57b716ed6..1e34b79954 100644
--- a/tests/auto/qml/qv4mm/tst_qv4mm.cpp
+++ b/tests/auto/qml/qv4mm/tst_qv4mm.cpp
@@ -112,26 +112,40 @@ void tst_qv4mm::accessParentOnDestruction()
 
 void tst_qv4mm::clearICParent()
 {
-    QJSEngine engine;
-    QJSValue value = engine.evaluate(
-        "(function() {\n"
-        "   var test = Object.create(null);\n"
-        "   for (var i = 0; i < 100; i++)\n"
-        "      test[(\"key_\"+i)] = true;\n"
-        "   for (var i = 0; i < 100; i++)\n"
-        "      delete test[\"key_\" + i];\n"
-        "   return test;\n"
-        "})();"
-    );
-    engine.collectGarbage();
-    QV4::Value *v4Value = QJSValuePrivate::getValue(&value);
-    QVERIFY(v4Value);
-    QV4::Heap::Object *v4Object = v4Value->toObject(engine.handle());
-    QVERIFY(v4Object);
-
-    // It should garbage collect the parents of the internalClass,
-    // as those aren't used anywhere else.
-    QCOMPARE(v4Object->internalClass->parent, nullptr);
+    QV4::ExecutionEngine engine;
+    QV4::Scope scope(engine.rootContext());
+    QV4::ScopedObject object(scope, engine.newObject());
+
+    // Keep identifiers in a separate array so that we don't have to allocate them in the loop that
+    // should test the GC on InternalClass allocations.
+    QV4::ScopedArrayObject identifiers(scope, engine.newArrayObject());
+    for (uint i = 0; i < 16 * 1024; ++i) {
+        QV4::Scope scope(&engine);
+        QV4::ScopedString s(scope);
+        s = engine.newIdentifier(QString::fromLatin1("key%1").arg(i));
+        identifiers->push_back(s);
+
+        QV4::ScopedValue v(scope);
+        v->setDouble(i);
+        object->insertMember(s, v);
+    }
+
+    // When allocating the InternalClass objects required for deleting properties, the GC should
+    // eventually run and remove all but the last two.
+    // If we ever manage to avoid allocating the InternalClasses in the first place we will need
+    // to change this test.
+    for (uint i = 0; i < 16 * 1024; ++i) {
+        QV4::Scope scope(&engine);
+        QV4::ScopedString s(scope, identifiers->getIndexed(i));
+        QV4::Scoped<QV4::InternalClass> ic(scope, object->internalClass());
+        QVERIFY(ic->d()->parent != nullptr);
+        object->deleteProperty(s->toPropertyKey());
+        QVERIFY(object->internalClass() != ic->d());
+        QCOMPARE(object->internalClass()->parent, ic->d());
+        if (ic->d()->parent == nullptr)
+            return;
+    }
+    QFAIL("Garbage collector was not triggered by large amount of InternalClasses");
 }
 
 QTEST_MAIN(tst_qv4mm)