improve memory management scheme of QScriptString(Private)

Get rid of QPointer.
Use linked list of privates (like was recently done for QScriptValue).
Allocate the private on the stack when we can.

Reviewed-by: Olivier Goffart

5 files changed, 119 insertions, 44 deletions

diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp
index 359d1e6..d85ecd2 100644
--- a/src/script/api/qscriptengine.cpp
+++ b/src/script/api/qscriptengine.cpp
@@ -791,7 +791,8 @@ static QScriptValue __setupPackage__(QScriptContext *ctx, QScriptEngine *eng)
 } // namespace QScript
 
 QScriptEnginePrivate::QScriptEnginePrivate()
-    : registeredScriptValues(0), freeScriptValues(0), inEval(false)
+    : registeredScriptValues(0), freeScriptValues(0),
+      registeredScriptStrings(0), inEval(false)
 {
     qMetaTypeId<QScriptValue>();
 
@@ -839,6 +840,7 @@ QScriptEnginePrivate::~QScriptEnginePrivate()
     while (!ownedAgents.isEmpty())
         delete ownedAgents.takeFirst();
     detachAllRegisteredScriptValues();
+    detachAllRegisteredScriptStrings();
     qDeleteAll(m_qobjectData);
     qDeleteAll(m_typeInfos);
     JSC::JSLock lock(false);
@@ -1352,6 +1354,19 @@ void QScriptEnginePrivate::detachAllRegisteredScriptValues()
     registeredScriptValues = 0;
 }
 
+void QScriptEnginePrivate::detachAllRegisteredScriptStrings()
+{
+    QScriptStringPrivate *it;
+    QScriptStringPrivate *next;
+    for (it = registeredScriptStrings; it != 0; it = next) {
+        it->detachFromEngine();
+        next = it->next;
+        it->prev = 0;
+        it->next = 0;
+    }
+    registeredScriptStrings = 0;
+}
+
 #ifdef QT_NO_QOBJECT
 
 QScriptEngine::QScriptEngine()
@@ -3621,7 +3636,11 @@ QScriptEngineAgent *QScriptEngine::agent() const
 QScriptString QScriptEngine::toStringHandle(const QString &str)
 {
     Q_D(QScriptEngine);
-    return d->scriptStringFromJSCIdentifier(JSC::Identifier(d->currentFrame, str));
+    QScriptString result;
+    QScriptStringPrivate *p = new QScriptStringPrivate(d, JSC::Identifier(d->currentFrame, str), QScriptStringPrivate::HeapAllocated);
+    QScriptStringPrivate::init(result, p);
+    d->registerScriptString(p);
+    return result;
 }
 
 /*!
diff --git a/src/script/api/qscriptengine_p.h b/src/script/api/qscriptengine_p.h
index f8eee87..826c2da 100644
--- a/src/script/api/qscriptengine_p.h
+++ b/src/script/api/qscriptengine_p.h
@@ -123,7 +123,6 @@ public:
 
     inline QScriptValue scriptValueFromJSCValue(JSC::JSValue value);
     inline JSC::JSValue scriptValueToJSCValue(const QScriptValue &value);
-    inline QScriptString scriptStringFromJSCIdentifier(const JSC::Identifier &id);
 
     QScriptValue scriptValueFromVariant(const QVariant &value);
     QVariant scriptValueToVariant(const QScriptValue &value, int targetType);
@@ -219,6 +218,10 @@ public:
     inline void unregisterScriptValue(QScriptValuePrivate *value);
     void detachAllRegisteredScriptValues();
 
+    inline void registerScriptString(QScriptStringPrivate *value);
+    inline void unregisterScriptString(QScriptStringPrivate *value);
+    void detachAllRegisteredScriptStrings();
+
     // private slots
     void _q_objectDestroyed(QObject *);
 #endif
@@ -243,6 +246,7 @@ public:
     int agentLineNumber;
     QScriptValuePrivate *registeredScriptValues;
     QScriptValuePrivate *freeScriptValues;
+    QScriptStringPrivate *registeredScriptStrings;
     QHash<int, QScriptTypeInfo*> m_typeInfos;
     int processEventsInterval;
     QScriptValue abortResult;
@@ -364,13 +368,28 @@ inline QScriptValue QScriptValuePrivate::property(const QString &name, int resol
     return property(JSC::Identifier(exec, name), resolveMode);
 }
 
-inline QScriptString QScriptEnginePrivate::scriptStringFromJSCIdentifier(const JSC::Identifier &id)
+inline void QScriptEnginePrivate::registerScriptString(QScriptStringPrivate *value)
 {
-    QScriptString q;
-    q.d_ptr = new QScriptStringPrivate(q_func(), id);
-    return q;
+    Q_ASSERT(value->type == QScriptStringPrivate::HeapAllocated);
+    value->prev = 0;
+    value->next = registeredScriptStrings;
+    if (registeredScriptStrings)
+        registeredScriptStrings->prev = value;
+    registeredScriptStrings = value;
 }
 
+inline void QScriptEnginePrivate::unregisterScriptString(QScriptStringPrivate *value)
+{
+    Q_ASSERT(value->type == QScriptStringPrivate::HeapAllocated);
+    if (value->prev)
+        value->prev->next = value->next;
+    if (value->next)
+        value->next->prev = value->prev;
+    if (value == registeredScriptStrings)
+        registeredScriptStrings = value->next;
+    value->prev = 0;
+    value->next = 0;
+}
 
 QT_END_NAMESPACE
 
diff --git a/src/script/api/qscriptstring.cpp b/src/script/api/qscriptstring.cpp
index 6de1d88..8c818e1 100644
--- a/src/script/api/qscriptstring.cpp
+++ b/src/script/api/qscriptstring.cpp
@@ -39,10 +39,10 @@
 **
 ****************************************************************************/
 
-#include "config.h"
 #include "qscriptstring.h"
-
 #include "qscriptstring_p.h"
+#include "qscriptengine.h"
+#include "qscriptengine_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -70,22 +70,6 @@ QT_BEGIN_NAMESPACE
 */
 
 /*!
-  \internal
-*/
-QScriptStringPrivate::QScriptStringPrivate(QScriptEngine *e, const JSC::Identifier &id)
-    : engine(e), identifier(id)
-{
-    ref = 0;
-}
-
-/*!
-  \internal
-*/
-QScriptStringPrivate::~QScriptStringPrivate()
-{
-}
-
-/*!
   Constructs an invalid QScriptString.
 */
 QScriptString::QScriptString()
@@ -99,6 +83,13 @@ QScriptString::QScriptString()
 QScriptString::QScriptString(const QScriptString &other)
     : d_ptr(other.d_ptr)
 {
+    if (d_func() && (d_func()->type == QScriptStringPrivate::StackAllocated)) {
+        Q_ASSERT(d_func()->ref != 1);
+        d_ptr.detach();
+        d_func()->ref = 1;
+        d_func()->type = QScriptStringPrivate::HeapAllocated;
+        d_func()->engine->registerScriptString(d_func());
+    }
 }
 
 /*!
@@ -106,6 +97,19 @@ QScriptString::QScriptString(const QScriptString &other)
 */
 QScriptString::~QScriptString()
 {
+    Q_D(QScriptString);
+    if (d) {
+        switch (d->type) {
+        case QScriptStringPrivate::StackAllocated:
+            Q_ASSERT(d->ref == 1);
+            d->ref.ref(); // avoid deletion
+            break;
+        case QScriptStringPrivate::HeapAllocated:
+            if (d->engine && (d->ref == 1))
+                d->engine->unregisterScriptString(d);
+            break;
+        }
+    }
 }
 
 /*!
@@ -113,7 +117,18 @@ QScriptString::~QScriptString()
 */
 QScriptString &QScriptString::operator=(const QScriptString &other)
 {
+    if (d_func() && d_func()->engine && (d_func()->ref == 1) && (d_func()->type == QScriptStringPrivate::HeapAllocated)) {
+        // current d_ptr will be deleted at the assignment below, so unregister it first
+        d_func()->engine->unregisterScriptString(d_func());
+    }
     d_ptr = other.d_ptr;
+    if (d_func() && (d_func()->type == QScriptStringPrivate::StackAllocated)) {
+        Q_ASSERT(d_func()->ref != 1);
+        d_ptr.detach();
+        d_func()->ref = 1;
+        d_func()->type = QScriptStringPrivate::HeapAllocated;
+        d_func()->engine->registerScriptString(d_func());
+    }
     return *this;
 }
 
@@ -137,7 +152,7 @@ bool QScriptString::operator==(const QScriptString &other) const
     if (d == other.d_func())
         return true;
     if (!d || !other.d_func())
-        return false;
+        return d == other.d_func();
     if (d->engine != other.d_func()->engine)
         return false;
     if (!d->engine)
diff --git a/src/script/api/qscriptstring.h b/src/script/api/qscriptstring.h
index e6224a2..2808916 100644
--- a/src/script/api/qscriptstring.h
+++ b/src/script/api/qscriptstring.h
@@ -52,10 +52,7 @@ QT_BEGIN_NAMESPACE
 
 QT_MODULE(Script)
 
-class QScriptEngine;
 class QScriptStringPrivate;
-struct QScriptStringPrivatePointerDeleter;
-
 class Q_SCRIPT_EXPORT QScriptString
 {
 public:
@@ -76,7 +73,6 @@ public:
 private:
     QExplicitlySharedDataPointer<QScriptStringPrivate> d_ptr;
     friend class QScriptValue;
-    friend class QScriptEnginePrivate;
     Q_DECLARE_PRIVATE(QScriptString)
 };
 
diff --git a/src/script/api/qscriptstring_p.h b/src/script/api/qscriptstring_p.h
index bba4b58..05fed77 100644
--- a/src/script/api/qscriptstring_p.h
+++ b/src/script/api/qscriptstring_p.h
@@ -55,32 +55,58 @@
 
 #include <QtCore/qobjectdefs.h>
 
-#include <QtCore/qstring.h>
-#include <QtCore/qpointer.h>
-#include "qscriptengine.h"
-
 #include "Identifier.h"
 
-
 QT_BEGIN_NAMESPACE
 
-class QScriptString;
-class QScriptEngine;
+class QScriptEnginePrivate;
 class QScriptStringPrivate
 {
 public:
-    QScriptStringPrivate(QScriptEngine *engine, const JSC::Identifier &id);
-    ~QScriptStringPrivate();
+    enum AllocationType {
+        StackAllocated,
+        HeapAllocated
+    };
+
+    inline QScriptStringPrivate(QScriptEnginePrivate *engine, const JSC::Identifier &id,
+                                AllocationType type);
+    inline ~QScriptStringPrivate();
+    static inline void init(QScriptString &q, QScriptStringPrivate *d);
+
+    inline void detachFromEngine();
 
     QBasicAtomicInt ref;
-#ifndef QT_NO_QOBJECT
-    QPointer<QScriptEngine> engine;
-#else
-    void *engine;
-#endif
+    QScriptEnginePrivate *engine;
     JSC::Identifier identifier;
+    AllocationType type;
+
+    // linked list of engine's script values
+    QScriptStringPrivate *prev;
+    QScriptStringPrivate *next;
 };
 
+inline QScriptStringPrivate::QScriptStringPrivate(QScriptEnginePrivate *e, const JSC::Identifier &id,
+                                                  AllocationType tp)
+    : engine(e), identifier(id), type(tp), prev(0), next(0)
+{
+    ref = 0;
+}
+
+inline QScriptStringPrivate::~QScriptStringPrivate()
+{
+}
+
+inline void QScriptStringPrivate::init(QScriptString &q, QScriptStringPrivate *d)
+{
+    q.d_ptr = d;
+}
+
+inline void QScriptStringPrivate::detachFromEngine()
+{
+    engine = 0;
+    identifier = JSC::Identifier();
+}
+
 QT_END_NAMESPACE
 
 #endif