From b765e3a84bc531878a5cc0d451451a94565b13f8 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C4=99drzej=20Nowacki?= Date: Thu, 19 Apr 2012 14:38:38 +0200 Subject: [PATCH] Avoid calling gc in QQmlEngine destructor. GC may be expensive. GC call in QQmlEngine doesn't have much sense because V8 will destroy all objects living in a current context. The only problem is that V8 may decide to not invoke weak callbacks which may cause a memory leak. To avoid that we track all QObjects that have JavaScript ownership set and we delete them explicitly. The change reduce time of destroying QQmlEngine by 75%, which is really visible in qquicklistmodel test. Change-Id: I2a3668fd23630669114baee8c241a7ecc4100e33 Reviewed-by: Chris Adams --- src/qml/qml/qqmlengine.cpp | 14 ++-- src/qml/qml/v8/qv8engine_p.h | 22 +----- src/qml/qml/v8/qv8objectresource_p.h | 84 ++++++++++++++++++++ src/qml/qml/v8/qv8qobjectwrapper.cpp | 56 +++++++------ src/qml/qml/v8/qv8qobjectwrapper_p.h | 27 ++++++ src/qml/qml/v8/v8.pri | 3 +- tests/auto/qml/qjsengine/tst_qjsengine.cpp | 14 +++ .../auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp | 16 ++-- tests/auto/qml/qqmlengine/tst_qqmlengine.cpp | 40 +++++++++- 9 files changed, 213 insertions(+), 63 deletions(-) create mode 100644 src/qml/qml/v8/qv8objectresource_p.h diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp index cef878a..626a4b0 100644 --- a/src/qml/qml/qqmlengine.cpp +++ b/src/qml/qml/qqmlengine.cpp @@ -592,9 +592,6 @@ QQmlEngine::~QQmlEngine() } } - // ensure we clean up QObjects with JS ownership - d->v8engine()->gc(); - if (d->incubationController) d->incubationController->d = 0; } @@ -899,10 +896,13 @@ void QQmlEngine::setContextForObject(QObject *object, QQmlContext *context) \value JavaScriptOwnership The object is owned by JavaScript. When the object is returned to QML as the return value of a method - call or property access, QML will delete the object if there are no - remaining JavaScript references to it and it has no - QObject::parent(). This option is similar to - QScriptEngine::ScriptOwnership. + call or property access, QML will track it, and delete the object + if there are no remaining JavaScript references to it and it has no + QObject::parent(). An object tracked by one QQmlEngine + will be deleted during that QQmlEngine's destructor, and thus + JavaScript references between objects with JavaScriptOwnership from + two different engines will not be valid after the deletion of one of + those engines. This option is similar to QScriptEngine::ScriptOwnership. Generally an application doesn't need to set an object's ownership explicitly. QML uses a heuristic to set the default object diff --git a/src/qml/qml/v8/qv8engine_p.h b/src/qml/qml/v8/qv8engine_p.h index 1fc03d8..09e1ae5 100644 --- a/src/qml/qml/v8/qv8engine_p.h +++ b/src/qml/qml/v8/qv8engine_p.h @@ -72,6 +72,7 @@ #include +#include "qv8objectresource_p.h" #include "qv8contextwrapper_p.h" #include "qv8qobjectwrapper_p.h" #include "qv8stringwrapper_p.h" @@ -91,12 +92,6 @@ QT_BEGIN_NAMESPACE // a handle, qFatal() is called. // #define QML_GLOBAL_HANDLE_DEBUGGING -#define V8_RESOURCE_TYPE(resourcetype) \ -public: \ - enum { V8ResourceType = QV8ObjectResource:: resourcetype }; \ - virtual QV8ObjectResource::ResourceType resourceType() const { return QV8ObjectResource:: resourcetype; } \ -private: - #define V8ENGINE() ((QV8Engine *)v8::External::Unwrap(args.Data())) #define V8FUNCTION(function, engine) v8::FunctionTemplate::New(function, v8::External::Wrap((QV8Engine*)engine))->GetFunction() #define V8THROW_ERROR(string) { \ @@ -132,21 +127,6 @@ private: } \ -class QV8Engine; -class QV8ObjectResource : public v8::Object::ExternalResource -{ -public: - QV8ObjectResource(QV8Engine *engine) : engine(engine) { Q_ASSERT(engine); } - enum ResourceType { ContextType, QObjectType, TypeType, ListType, VariantType, - ValueTypeType, XMLHttpRequestType, DOMNodeType, SQLDatabaseType, - ListModelType, Context2DType, Context2DStyleType, Context2DPixelArrayType, - ParticleDataType, SignalHandlerType, IncubatorType, VisualDataItemType, - SequenceType, LocaleDataType }; - virtual ResourceType resourceType() const = 0; - - QV8Engine *engine; -}; - template inline T *v8_resource_cast(v8::Handle object) { QV8ObjectResource *resource = static_cast(object->GetExternalResource()); diff --git a/src/qml/qml/v8/qv8objectresource_p.h b/src/qml/qml/v8/qv8objectresource_p.h new file mode 100644 index 0000000..c37905a --- /dev/null +++ b/src/qml/qml/v8/qv8objectresource_p.h @@ -0,0 +1,84 @@ +/**************************************************************************** +** +** Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies). +** Contact: http://www.qt-project.org/ +** +** This file is part of the QtQml module of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** GNU Lesser General Public License Usage +** This file may be used under the terms of the GNU Lesser General Public +** License version 2.1 as published by the Free Software Foundation and +** appearing in the file LICENSE.LGPL included in the packaging of this +** file. Please review the following information to ensure the GNU Lesser +** General Public License version 2.1 requirements will be met: +** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, Nokia gives you certain additional +** rights. These rights are described in the Nokia Qt LGPL Exception +** version 1.1, included in the file LGPL_EXCEPTION.txt in this package. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU General +** Public License version 3.0 as published by the Free Software Foundation +** and appearing in the file LICENSE.GPL included in the packaging of this +** file. Please review the following information to ensure the GNU General +** Public License version 3.0 requirements will be met: +** http://www.gnu.org/copyleft/gpl.html. +** +** Other Usage +** Alternatively, this file may be used in accordance with the terms and +** conditions contained in a signed written agreement between you and Nokia. +** +** +** +** +** +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#ifndef QV8OBJECTRESOURCE_P_H +#define QV8OBJECTRESOURCE_P_H + +// +// W A R N I N G +// ------------- +// +// This file is not part of the Qt API. It exists purely as an +// implementation detail. This header file may change from version to +// version without notice, or even be removed. +// +// We mean it. +// + +#include +#include + +QT_BEGIN_NAMESPACE + +#define V8_RESOURCE_TYPE(resourcetype) \ +public: \ + enum { V8ResourceType = QV8ObjectResource:: resourcetype }; \ + virtual QV8ObjectResource::ResourceType resourceType() const { return QV8ObjectResource:: resourcetype; } \ +private: + +class QV8Engine; +class QV8ObjectResource : public v8::Object::ExternalResource +{ +public: + QV8ObjectResource(QV8Engine *engine) : engine(engine) { Q_ASSERT(engine); } + enum ResourceType { ContextType, QObjectType, TypeType, ListType, VariantType, + ValueTypeType, XMLHttpRequestType, DOMNodeType, SQLDatabaseType, + ListModelType, Context2DType, Context2DStyleType, Context2DPixelArrayType, + ParticleDataType, SignalHandlerType, IncubatorType, VisualDataItemType, + SequenceType, LocaleDataType }; + virtual ResourceType resourceType() const = 0; + + QV8Engine *engine; +}; + +QT_END_NAMESPACE + +#endif // QV8OBJECTRESOURCE_P_H diff --git a/src/qml/qml/v8/qv8qobjectwrapper.cpp b/src/qml/qml/v8/qv8qobjectwrapper.cpp index 70f10b2..2d6c5ec 100644 --- a/src/qml/qml/v8/qv8qobjectwrapper.cpp +++ b/src/qml/qml/v8/qv8qobjectwrapper.cpp @@ -80,16 +80,6 @@ QT_BEGIN_NAMESPACE // XXX TODO: Need to review all calls to QQmlEngine *engine() to confirm QObjects work // correctly in a worker thread -class QV8QObjectResource : public QV8ObjectResource -{ - V8_RESOURCE_TYPE(QObjectType); - -public: - QV8QObjectResource(QV8Engine *engine, QObject *object); - - QQmlGuard object; -}; - class QV8QObjectInstance : public QQmlGuard { public: @@ -223,6 +213,13 @@ void QV8QObjectWrapper::destroy() qPersistentDispose(m_signalHandlerConstructor); qPersistentDispose(m_methodConstructor); qPersistentDispose(m_constructor); + + QIntrusiveList::iterator i = m_javaScriptOwnedWeakQObjects.begin(); + for (; i != m_javaScriptOwnedWeakQObjects.end(); ++i) { + QV8QObjectResource *resource = *i; + Q_ASSERT(resource); + deleteWeakQObject(resource); + } } struct ReadAccessor { @@ -930,25 +927,14 @@ static void FastValueSetterReadOnly(v8::Local property, v8::LocaltoString(error))); } -static void WeakQObjectReferenceCallback(v8::Persistent handle, void *) +void QV8QObjectWrapper::WeakQObjectReferenceCallback(v8::Persistent handle, void *wrapper) { Q_ASSERT(handle->IsObject()); - QV8QObjectResource *resource = v8_resource_check(handle->ToObject()); - Q_ASSERT(resource); - QObject *object = resource->object; - if (object) { - QQmlData *ddata = QQmlData::get(object, false); - if (ddata) { - ddata->v8object.Clear(); - if (!object->parent() && !ddata->indestructible) { - ddata->isQueuedForDeletion = true; - object->deleteLater(); - } - } - } + static_cast(wrapper)->unregisterWeakQObjectReference(resource); + static_cast(wrapper)->deleteWeakQObject(resource); qPersistentDispose(handle); } @@ -1117,8 +1103,10 @@ v8::Handle QV8QObjectWrapper::newQObject(QObject *object) v8::Local rv = newQObject(object, ddata, m_engine); ddata->v8object = qPersistentNew(rv); - ddata->v8object.MakeWeak(0, WeakQObjectReferenceCallback); + ddata->v8object.MakeWeak(this, WeakQObjectReferenceCallback); ddata->v8objectid = m_id; + QV8QObjectResource *resource = v8_resource_check(rv); + registerWeakQObjectReference(resource); return rv; } else { @@ -1133,8 +1121,10 @@ v8::Handle QV8QObjectWrapper::newQObject(QObject *object) if ((!found || (*iter)->v8object.IsEmpty()) && ddata->v8object.IsEmpty()) { v8::Local rv = newQObject(object, ddata, m_engine); ddata->v8object = qPersistentNew(rv); - ddata->v8object.MakeWeak(0, WeakQObjectReferenceCallback); + ddata->v8object.MakeWeak(this, WeakQObjectReferenceCallback); ddata->v8objectid = m_id; + QV8QObjectResource *resource = v8_resource_check(rv); + registerWeakQObjectReference(resource); if (found) { delete (*iter); @@ -1157,6 +1147,20 @@ v8::Handle QV8QObjectWrapper::newQObject(QObject *object) return v8::Local::New((*iter)->v8object); } } +void QV8QObjectWrapper::deleteWeakQObject(QV8QObjectResource *resource) +{ + QObject *object = resource->object; + if (object) { + QQmlData *ddata = QQmlData::get(object, false); + if (ddata) { + ddata->v8object.Clear(); + if (!object->parent() && !ddata->indestructible) { + ddata->isQueuedForDeletion = true; + object->deleteLater(); + } + } + } +} QPair QV8QObjectWrapper::ExtractQtSignal(QV8Engine *engine, v8::Handle object) { diff --git a/src/qml/qml/v8/qv8qobjectwrapper_p.h b/src/qml/qml/v8/qv8qobjectwrapper_p.h index f7b9656..ab037ee 100644 --- a/src/qml/qml/v8/qv8qobjectwrapper_p.h +++ b/src/qml/qml/v8/qv8qobjectwrapper_p.h @@ -61,6 +61,8 @@ #include #include #include +#include +#include "qv8objectresource_p.h" QT_BEGIN_NAMESPACE @@ -71,6 +73,18 @@ class QV8ObjectResource; class QV8QObjectInstance; class QV8QObjectConnectionList; class QQmlPropertyCache; + +class QV8QObjectResource : public QV8ObjectResource +{ + V8_RESOURCE_TYPE(QObjectType); + +public: + QV8QObjectResource(QV8Engine *engine, QObject *object); + + QQmlGuard object; + QIntrusiveListNode weakResource; +}; + class Q_QML_EXPORT QV8QObjectWrapper { public: @@ -89,12 +103,23 @@ public: inline v8::Handle getProperty(QObject *, const QHashedV8String &, RevisionMode); inline bool setProperty(QObject *, const QHashedV8String &, v8::Handle, RevisionMode); + void registerWeakQObjectReference(QV8QObjectResource *resource) + { + m_javaScriptOwnedWeakQObjects.insert(resource); + } + + void unregisterWeakQObjectReference(QV8QObjectResource *resource) + { + m_javaScriptOwnedWeakQObjects.remove(resource); + } + private: friend class QQmlPropertyCache; friend class QV8QObjectConnectionList; friend class QV8QObjectInstance; v8::Local newQObject(QObject *, QQmlData *, QV8Engine *); + void deleteWeakQObject(QV8QObjectResource *resource); static v8::Handle GetProperty(QV8Engine *, QObject *, v8::Handle *, const QHashedV8String &, QV8QObjectWrapper::RevisionMode); static bool SetProperty(QV8Engine *, QObject *, const QHashedV8String &, @@ -112,6 +137,7 @@ private: static v8::Handle Invoke(const v8::Arguments &args); static QPair ExtractQtMethod(QV8Engine *, v8::Handle); static QPair ExtractQtSignal(QV8Engine *, v8::Handle); + static void WeakQObjectReferenceCallback(v8::Persistent handle, void *wrapper); QV8Engine *m_engine; quint32 m_id; @@ -126,6 +152,7 @@ private: QHash m_connections; typedef QHash TaintedHash; TaintedHash m_taintedObjects; + QIntrusiveList m_javaScriptOwnedWeakQObjects; }; v8::Handle QV8QObjectWrapper::getProperty(QObject *object, const QHashedV8String &string, diff --git a/src/qml/qml/v8/v8.pri b/src/qml/qml/v8/v8.pri index 52b6bf4..33a0ad1 100644 --- a/src/qml/qml/v8/v8.pri +++ b/src/qml/qml/v8/v8.pri @@ -22,7 +22,8 @@ HEADERS += \ $$PWD/qv8engine_impl_p.h \ $$PWD/qv8domerrors_p.h \ $$PWD/qv8sqlerrors_p.h \ - $$PWD/qqmlbuiltinfunctions_p.h + $$PWD/qqmlbuiltinfunctions_p.h \ + $$PWD/qv8objectresource_p.h SOURCES += \ $$PWD/qv8stringwrapper.cpp \ diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp index e34304b..5ebc42a 100644 --- a/tests/auto/qml/qjsengine/tst_qjsengine.cpp +++ b/tests/auto/qml/qjsengine/tst_qjsengine.cpp @@ -120,6 +120,7 @@ private slots: #endif void newQObject_promoteNonObject(); void newQObject_promoteNonQScriptObject(); + void newQObject_deletedEngine(); #if 0 // ### FIXME: No QScript Metaobject support right now void newQMetaObject(); void newActivationObject(); @@ -1120,6 +1121,19 @@ void tst_QJSEngine::newQObject_promoteNonQScriptObject() #endif } +void tst_QJSEngine::newQObject_deletedEngine() +{ + QJSValue object; + QObject *ptr = new QObject(); + QSignalSpy spy(ptr, SIGNAL(destroyed())); + { + QJSEngine engine; + object = engine.newQObject(ptr); + engine.globalObject().setProperty("obj", object); + } + QTRY_VERIFY(spy.count()); +} + #if 0 // ### FIXME: No QScript Metaobject support right now QT_BEGIN_NAMESPACE Q_SCRIPT_DECLARE_QMETAOBJECT(QObject, QObject*) diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp index 4b913ef..cb56948 100644 --- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp +++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp @@ -4904,18 +4904,20 @@ void tst_qqmlecmascript::handleReferenceManagement() QQmlEngine::setObjectOwnership(second1, QQmlEngine::JavaScriptOwnership); QQmlEngine::setObjectOwnership(first2, QQmlEngine::JavaScriptOwnership); QQmlEngine::setObjectOwnership(second2, QQmlEngine::JavaScriptOwnership); - gc(engine); - QCOMPARE(dtorCount, 0); - delete hrmEngine2; - gc(engine); + gc(*hrmEngine1); + gc(*hrmEngine2); QCOMPARE(dtorCount, 0); + delete hrmEngine2; // should trigger deletion of objects with JS ownership tracked by this engine + gc(*hrmEngine1); + QCOMPARE(dtorCount, 2); // first2 and second2 should have been deleted. delete object1; delete object2; - hrmEngine1->collectGarbage(); + gc(*hrmEngine1); + QCOMPARE(dtorCount, 6); // deleting object1 and object2 should trigger deletion of first1 and first2. + delete hrmEngine1; QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete); QCoreApplication::processEvents(); - QCOMPARE(dtorCount, 6); - delete hrmEngine1; + QCOMPARE(dtorCount, 6); // all objects should have been cleaned up prior to deleting hrmEngine1. } } diff --git a/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp b/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp index c039429..ab18cdd 100644 --- a/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp +++ b/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,13 @@ private slots: void outputWarningsToStandardError(); void objectOwnership(); void multipleEngines(); + +public slots: + QObject *createAQObjectForOwnershipTest () + { + static QObject *ptr = new QObject(); + return ptr; + } }; void tst_qqmlengine::rootContext() @@ -326,7 +334,37 @@ void tst_qqmlengine::objectOwnership() delete o; } - + { + QObject *ptr = createAQObjectForOwnershipTest(); + QSignalSpy spy(ptr, SIGNAL(destroyed())); + { + QQmlEngine engine; + QQmlComponent c(&engine); + engine.rootContext()->setContextProperty("test", this); + QQmlEngine::setObjectOwnership(ptr, QQmlEngine::JavaScriptOwnership); + c.setData("import QtQuick 2.0; Item { property int data: test.createAQObjectForOwnershipTest() ? 0 : 1 }", QUrl()); + QVERIFY(c.isReady()); + QObject *o = c.create(); + QVERIFY(o != 0); + } + QTRY_VERIFY(spy.count()); + } + { + QObject *ptr = new QObject(); + QSignalSpy spy(ptr, SIGNAL(destroyed())); + { + QQmlEngine engine; + QQmlComponent c(&engine); + engine.rootContext()->setContextProperty("test", ptr); + QQmlEngine::setObjectOwnership(ptr, QQmlEngine::JavaScriptOwnership); + c.setData("import QtQuick 2.0; QtObject { property var object: { var i = test; test ? 0 : 1 } }", QUrl()); + QVERIFY(c.isReady()); + QObject *o = c.create(); + QVERIFY(o != 0); + engine.rootContext()->setContextProperty("test", 0); + } + QTRY_VERIFY(spy.count()); + } } // Test an object can be accessed by multiple engines -- 1.7.2.5