From b9fd1367ab71813e15a7b25733bd9fdb0ca6d7aa Mon Sep 17 00:00:00 2001 From: Jedrzej Nowacki Date: Tue, 2 Aug 2011 13:14:26 +0200 Subject: [PATCH] Improve QJSValueIterator implementation. The old implementation was a hack, it had some memory leak (in case of deleted engine) and performance problems (for example all names were copied to separate QList instance instead of reusing v8::Array). Change-Id: Ic70ad511127a8c05df3c627e4496083004c6452a Reviewed-on: http://codereview.qt.nokia.com/2512 Reviewed-by: Qt Sanity Bot Reviewed-by: Kent Hansen --- src/declarative/qml/v8/qjsvalue_p.h | 2 + src/declarative/qml/v8/qjsvalueiterator.cpp | 138 +--------------------- src/declarative/qml/v8/qjsvalueiterator_impl_p.h | 121 +++++++++++++++++++ src/declarative/qml/v8/qjsvalueiterator_p.h | 64 ++++++++++ src/declarative/qml/v8/qscript_impl_p.h | 1 + src/declarative/qml/v8/qv8engine_impl_p.h | 22 ++++ src/declarative/qml/v8/qv8engine_p.h | 7 + src/declarative/qml/v8/script.pri | 4 +- 8 files changed, 221 insertions(+), 138 deletions(-) create mode 100644 src/declarative/qml/v8/qjsvalueiterator_impl_p.h create mode 100644 src/declarative/qml/v8/qjsvalueiterator_p.h diff --git a/src/declarative/qml/v8/qjsvalue_p.h b/src/declarative/qml/v8/qjsvalue_p.h index 7b2a001..2e24ad0 100644 --- a/src/declarative/qml/v8/qjsvalue_p.h +++ b/src/declarative/qml/v8/qjsvalue_p.h @@ -49,6 +49,8 @@ QT_BEGIN_NAMESPACE +class QV8Engine; + /*! \internal \class QJSValuePrivate diff --git a/src/declarative/qml/v8/qjsvalueiterator.cpp b/src/declarative/qml/v8/qjsvalueiterator.cpp index ca9123f..42bfd34 100644 --- a/src/declarative/qml/v8/qjsvalueiterator.cpp +++ b/src/declarative/qml/v8/qjsvalueiterator.cpp @@ -22,6 +22,7 @@ ****************************************************************************/ #include "qjsvalueiterator.h" +#include "qjsvalueiterator_p.h" #include "qscriptisolate_p.h" #include "qjsvalue_p.h" @@ -69,143 +70,6 @@ QT_BEGIN_NAMESPACE \sa QJSValue::property() */ -using v8::Persistent; -using v8::Local; -using v8::Array; -using v8::String; -using v8::Handle; -using v8::Object; -using v8::Value; - -// FIXME (Qt5) This class should be refactored. It should use the common Iterator interface. -// FIXME it could be faster! -class QJSValueIteratorPrivate { -public: - inline QJSValueIteratorPrivate(const QJSValuePrivate* value); - inline ~QJSValueIteratorPrivate(); - - inline bool hasNext() const; - inline bool next(); - - inline QString name() const; - - inline QScriptPassPointer value() const; - - inline bool isValid() const; - inline QV8Engine* engine() const; -private: - Q_DISABLE_COPY(QJSValueIteratorPrivate) - //void dump(QString) const; - - QScriptSharedDataPointer m_object; - QList > m_names; - QMutableListIterator > m_iterator; -}; - -inline QJSValueIteratorPrivate::QJSValueIteratorPrivate(const QJSValuePrivate* value) - : m_object(const_cast(value)) - , m_iterator(m_names) -{ - Q_ASSERT(value); - QV8Engine *engine = m_object->engine(); - QScriptIsolate api(engine); - if (!m_object->isObject()) - m_object = 0; - else { - v8::HandleScope scope; - Handle tmp = *value; - Handle obj = Handle::Cast(tmp); - Local names; - - // FIXME we need newer V8! - //names = obj->GetOwnPropertyNames(); - names = engine->getOwnPropertyNames(obj); - - // it is suboptimal, it would be better to write iterator instead - uint32_t count = names->Length(); - Local name; - m_names.reserve(count); // The count is the maximal count of values. - for (uint32_t i = count - 1; i < count; --i) { - name = names->Get(i)->ToString(); - m_names.append(v8::Persistent::New(name)); - } - - // Reinitialize the iterator. - m_iterator = m_names; - } -} - -inline QJSValueIteratorPrivate::~QJSValueIteratorPrivate() -{ - QMutableListIterator > it = m_names; - //FIXME: we need register this QJSVAlueIterator - if (engine()) { - while (it.hasNext()) { - it.next().Dispose(); - } - } else { - // FIXME leak ? - } -} - -inline bool QJSValueIteratorPrivate::hasNext() const -{ - //dump("hasNext()"); - return isValid() - ? m_iterator.hasNext() : false; -} - -inline bool QJSValueIteratorPrivate::next() -{ - //dump("next();"); - if (m_iterator.hasNext()) { - m_iterator.next(); - return true; - } - return false; -} - -inline QString QJSValueIteratorPrivate::name() const -{ - //dump("name"); - if (!isValid()) - return QString(); - - return QJSConverter::toString(m_iterator.value()); -} - -inline QScriptPassPointer QJSValueIteratorPrivate::value() const -{ - //dump("value()"); - if (!isValid()) - return InvalidValue(); - - return m_object->property(m_iterator.value()); -} - -inline bool QJSValueIteratorPrivate::isValid() const -{ - bool result = m_object ? m_object->isValid() : false; - // We know that if this object is still valid then it is an object - // if this assumption is not correct then some other logic in this class - // have to be changed too. - Q_ASSERT(!result || m_object->isObject()); - return result; -} - -inline QV8Engine* QJSValueIteratorPrivate::engine() const -{ - return m_object ? m_object->engine() : 0; -} - -//void QJSValueIteratorPrivate::dump(QString fname) const -//{ -// qDebug() << " *** " << fname << " ***"; -// foreach (Persistent name, m_names) { -// qDebug() << " - " << QJSConverter::toString(name); -// } -//} - /*! Constructs an iterator for traversing \a object. The iterator is set to be at the front of the sequence of properties (before the diff --git a/src/declarative/qml/v8/qjsvalueiterator_impl_p.h b/src/declarative/qml/v8/qjsvalueiterator_impl_p.h new file mode 100644 index 0000000..68075e5 --- /dev/null +++ b/src/declarative/qml/v8/qjsvalueiterator_impl_p.h @@ -0,0 +1,121 @@ +/**************************************************************************** +** +** Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies). +** All rights reserved. +** Contact: Nokia Corporation (qt-info@nokia.com) +** +** This file is part of the QtDeclarative module of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL-ONLY$ +** 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. +** +** If you have questions regarding the use of this file, please contact +** Nokia at qt-info@nokia.com. +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#ifndef QJSVALUEITERATOR_IMPL_P_H +#define QJSVALUEITERATOR_IMPL_P_H + +#include +#include +#include + +inline QJSValueIteratorPrivate::QJSValueIteratorPrivate(const QJSValuePrivate* value) + : m_object(const_cast(value)) + , m_index(0) + , m_count(0) +{ + Q_ASSERT(value); + QV8Engine *engine = m_object->engine(); + if (!m_object->isObject()) + m_object = 0; + else { + QScriptIsolate api(engine, QScriptIsolate::NotNullEngine); + v8::HandleScope scope; + + v8::Handle tmp = *value; + v8::Handle obj = v8::Handle::Cast(tmp); + v8::Local names; + + // FIXME we need newer V8! + //names = obj->GetOwnPropertyNames(); + names = engine->getOwnPropertyNames(obj); + m_names = v8::Persistent::New(names); + m_count = names->Length(); + + engine->registerValueIterator(this); + } +} + +inline QJSValueIteratorPrivate::~QJSValueIteratorPrivate() +{ + if (isValid()) { + engine()->unregisterValueIterator(this); + m_names.Dispose(); + } +} + +inline void QJSValueIteratorPrivate::invalidate() +{ + m_names.Dispose(); + m_object.reset(); + m_index = 0; + m_count = 0; +} + +inline bool QJSValueIteratorPrivate::hasNext() const +{ + return isValid() ? m_index < m_count : false; +} + +inline bool QJSValueIteratorPrivate::next() +{ + if (hasNext()) { + ++m_index; + return true; + } + return false; +} + +inline QString QJSValueIteratorPrivate::name() const +{ + if (!isValid()) + return QString(); + + v8::HandleScope handleScope; + return QJSConverter::toString(m_names->Get(m_index - 1)->ToString()); +} + +inline QScriptPassPointer QJSValueIteratorPrivate::value() const +{ + if (!isValid()) + return InvalidValue(); + + v8::HandleScope handleScope; + return m_object->property(m_names->Get(m_index - 1)->ToString()); +} + +inline bool QJSValueIteratorPrivate::isValid() const +{ + bool result = m_object ? m_object->isValid() : false; + // We know that if this object is still valid then it is an object + // if this assumption is not correct then some other logic in this class + // have to be changed too. + Q_ASSERT(!result || m_object->isObject()); + return result; +} + +inline QV8Engine* QJSValueIteratorPrivate::engine() const +{ + return m_object ? m_object->engine() : 0; +} + +#endif // QJSVALUEITERATOR_IMPL_P_H diff --git a/src/declarative/qml/v8/qjsvalueiterator_p.h b/src/declarative/qml/v8/qjsvalueiterator_p.h new file mode 100644 index 0000000..d3033d4 --- /dev/null +++ b/src/declarative/qml/v8/qjsvalueiterator_p.h @@ -0,0 +1,64 @@ +/**************************************************************************** +** +** Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies). +** All rights reserved. +** Contact: Nokia Corporation (qt-info@nokia.com) +** +** This file is part of the QtDeclarative module of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL-ONLY$ +** 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. +** +** If you have questions regarding the use of this file, please contact +** Nokia at qt-info@nokia.com. +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#ifndef QJSVALUEITERATOR_P_H +#define QJSVALUEITERATOR_P_H + +#include +#include + +#include + +QT_BEGIN_NAMESPACE + +class QV8Engine; + +class QJSValueIteratorPrivate : public QScriptLinkedNode { +public: + inline QJSValueIteratorPrivate(const QJSValuePrivate* value); + inline ~QJSValueIteratorPrivate(); + + inline bool hasNext() const; + inline bool next(); + + inline QString name() const; + + inline QScriptPassPointer value() const; + + inline bool isValid() const; + inline QV8Engine* engine() const; + + inline void invalidate(); +private: + Q_DISABLE_COPY(QJSValueIteratorPrivate) + + QScriptSharedDataPointer m_object; + v8::Persistent m_names; + uint32_t m_index; + uint32_t m_count; +}; + + +QT_END_NAMESPACE + +#endif // QJSVALUEITERATOR_P_H diff --git a/src/declarative/qml/v8/qscript_impl_p.h b/src/declarative/qml/v8/qscript_impl_p.h index e66b561..d197d9f 100644 --- a/src/declarative/qml/v8/qscript_impl_p.h +++ b/src/declarative/qml/v8/qscript_impl_p.h @@ -37,5 +37,6 @@ #include "qv8engine_impl_p.h" #include "qjsvalue_impl_p.h" +#include "qjsvalueiterator_impl_p.h" #endif //QSCRIPT_IMPL_P_H diff --git a/src/declarative/qml/v8/qv8engine_impl_p.h b/src/declarative/qml/v8/qv8engine_impl_p.h index 5c56efd..382e221 100644 --- a/src/declarative/qml/v8/qv8engine_impl_p.h +++ b/src/declarative/qml/v8/qv8engine_impl_p.h @@ -38,6 +38,7 @@ #include "qv8engine_p.h" #include "qjsvalue_p.h" #include "qjsconverter_p.h" +#include "qjsvalueiterator_p.h" QT_BEGIN_NAMESPACE @@ -80,6 +81,10 @@ public: { value->reinitialize(); } + void operator () (QJSValueIteratorPrivate *iterator) const + { + iterator->invalidate(); + } }; inline void QV8Engine::registerValue(QJSValuePrivate *data) @@ -99,6 +104,23 @@ inline void QV8Engine::invalidateAllValues() m_values.clear(); } +inline void QV8Engine::registerValueIterator(QJSValueIteratorPrivate *data) +{ + m_valueIterators.insert(data); +} + +inline void QV8Engine::unregisterValueIterator(QJSValueIteratorPrivate *data) +{ + m_valueIterators.remove(data); +} + +inline void QV8Engine::invalidateAllIterators() +{ + QtScriptBagCleaner invalidator; + m_valueIterators.forEach(invalidator); + m_valueIterators.clear(); +} + /*! \internal \note property can be index (v8::Integer) or a property (v8::String) name, according to ECMA script diff --git a/src/declarative/qml/v8/qv8engine_p.h b/src/declarative/qml/v8/qv8engine_p.h index 938454c..9258a80 100644 --- a/src/declarative/qml/v8/qv8engine_p.h +++ b/src/declarative/qml/v8/qv8engine_p.h @@ -214,6 +214,8 @@ class QDeclarativeEngine; class QDeclarativeValueType; class QNetworkAccessManager; class QDeclarativeContextData; +class QJSValueIteratorPrivate; + class Q_DECLARATIVE_EXPORT QV8Engine { public: @@ -260,6 +262,10 @@ public: inline void unregisterValue(QJSValuePrivate *data); inline void invalidateAllValues(); + inline void registerValueIterator(QJSValueIteratorPrivate *data); + inline void unregisterValueIterator(QJSValueIteratorPrivate *data); + inline void invalidateAllIterators(); + QV8ContextWrapper *contextWrapper() { return &m_contextWrapper; } QV8QObjectWrapper *qobjectWrapper() { return &m_qobjectWrapper; } QV8TypeWrapper *typeWrapper() { return &m_typeWrapper; } @@ -455,6 +461,7 @@ protected: QDateTime qtDateTimeFromJsDate(double jsDate); private: QScriptBagContainer m_values; + QScriptBagContainer m_valueIterators; Q_DISABLE_COPY(QV8Engine) }; diff --git a/src/declarative/qml/v8/script.pri b/src/declarative/qml/v8/script.pri index 04a23d1..7454d20 100644 --- a/src/declarative/qml/v8/script.pri +++ b/src/declarative/qml/v8/script.pri @@ -17,4 +17,6 @@ HEADERS += \ $$PWD/qscriptshareddata_p.h \ $$PWD/qscripttools_p.h \ $$PWD/qscript_impl_p.h \ - $$PWD/qscriptoriginalglobalobject_p.h + $$PWD/qscriptoriginalglobalobject_p.h \ + $$PWD/qjsvalueiterator_p.h \ + $$PWD/qjsvalueiterator_impl_p.h -- 1.7.2.5