Unreviewed, rolling out r126287.
authorossy@webkit.org <ossy@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Aug 2012 14:13:19 +0000 (14:13 +0000)
committerossy@webkit.org <ossy@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Aug 2012 14:13:19 +0000 (14:13 +0000)
http://trac.webkit.org/changeset/126287
https://bugs.webkit.org/show_bug.cgi?id=94708

It made WK1 layout testing 3.7x slower (>1hours) (Requested by
ossy on #webkit).

Patch by Sheriff Bot <webkit.review.bot@gmail.com> on 2012-08-22

Source/WebCore:

* bridge/qt/qt_instance.cpp:
(JSC::Bindings::unusedWeakObjectMapCallback):
(Bindings):
(JSC::Bindings::WeakMapImpl::WeakMapImpl):
(JSC::Bindings::WeakMapImpl::~WeakMapImpl):
(JSC::Bindings::WeakMap::~WeakMap):
(JSC::Bindings::WeakMap::set):
(JSC::Bindings::WeakMap::get):
(JSC::Bindings::WeakMap::remove):
* bridge/qt/qt_instance.h:
(QtInstance):
* bridge/qt/qt_runtime.cpp:
(JSC::Bindings::prototypeForSignalsAndSlots):
(JSC::Bindings::QtRuntimeMethod::~QtRuntimeMethod):
(JSC::Bindings::QtRuntimeMethod::call):
(JSC::Bindings::QtRuntimeMethod::jsObjectRef):
(JSC::Bindings::QtRuntimeMethod::connectOrDisconnect):
* bridge/qt/qt_runtime.h:
(QtRuntimeMethod):

Source/WebKit/qt:

* tests/qobjectbridge/tst_qobjectbridge.cpp:
(tst_QObjectBridge::objectDeleted):
(tst_QObjectBridge::introspectQtMethods_data):
(tst_QObjectBridge::introspectQtMethods):

LayoutTests:

* platform/qt/Skipped:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@126301 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/qt/Skipped
Source/WebCore/ChangeLog
Source/WebCore/bridge/qt/qt_instance.cpp
Source/WebCore/bridge/qt/qt_instance.h
Source/WebCore/bridge/qt/qt_runtime.cpp
Source/WebCore/bridge/qt/qt_runtime.h
Source/WebKit/qt/ChangeLog
Source/WebKit/qt/tests/qobjectbridge/tst_qobjectbridge.cpp

index f81bf41..443ce32 100644 (file)
@@ -1,3 +1,14 @@
+2012-08-22  Sheriff Bot  <webkit.review.bot@gmail.com>
+
+        Unreviewed, rolling out r126287.
+        http://trac.webkit.org/changeset/126287
+        https://bugs.webkit.org/show_bug.cgi?id=94708
+
+        It made WK1 layout testing 3.7x slower (>1hours) (Requested by
+        ossy on #webkit).
+
+        * platform/qt/Skipped:
+
 2012-08-22  KwangYong Choi  <ky0.choi@samsung.com>
 
         [EFL] Support slider tick mark snapping
index 6bc5007..e824a67 100644 (file)
@@ -2730,6 +2730,9 @@ fast/js/dfg-compare-final-object-to-final-object-or-other-when-both-proven-final
 # https://bugs.webkit.org/show_bug.cgi?id=93812
 svg/custom/use-instanceRoot-as-event-target.xhtml
 
+# https://bugs.webkit.org/show_bug.cgi?id=93897
+fast/profiler/nested-start-and-stop-profiler.html
+
 # New test introduced in r125648 fast/events/autoscroll-in-textarea.html fails
 # https://bugs.webkit.org/show_bug.cgi?id=94076
 fast/events/autoscroll-in-textarea.html
index d98d6b4..883f4d6 100644 (file)
@@ -1,3 +1,32 @@
+2012-08-22  Sheriff Bot  <webkit.review.bot@gmail.com>
+
+        Unreviewed, rolling out r126287.
+        http://trac.webkit.org/changeset/126287
+        https://bugs.webkit.org/show_bug.cgi?id=94708
+
+        It made WK1 layout testing 3.7x slower (>1hours) (Requested by
+        ossy on #webkit).
+
+        * bridge/qt/qt_instance.cpp:
+        (JSC::Bindings::unusedWeakObjectMapCallback):
+        (Bindings):
+        (JSC::Bindings::WeakMapImpl::WeakMapImpl):
+        (JSC::Bindings::WeakMapImpl::~WeakMapImpl):
+        (JSC::Bindings::WeakMap::~WeakMap):
+        (JSC::Bindings::WeakMap::set):
+        (JSC::Bindings::WeakMap::get):
+        (JSC::Bindings::WeakMap::remove):
+        * bridge/qt/qt_instance.h:
+        (QtInstance):
+        * bridge/qt/qt_runtime.cpp:
+        (JSC::Bindings::prototypeForSignalsAndSlots):
+        (JSC::Bindings::QtRuntimeMethod::~QtRuntimeMethod):
+        (JSC::Bindings::QtRuntimeMethod::call):
+        (JSC::Bindings::QtRuntimeMethod::jsObjectRef):
+        (JSC::Bindings::QtRuntimeMethod::connectOrDisconnect):
+        * bridge/qt/qt_runtime.h:
+        (QtRuntimeMethod):
+
 2012-08-22  Pavel Feldman  <pfeldman@chromium.org>
 
         Not reviewed: follow up to r126297, fixing WebCore.gypi.
index 0412532..b9a34ac 100644 (file)
 namespace JSC {
 namespace Bindings {
 
+static void unusedWeakObjectMapCallback(JSWeakObjectMapRef, void*)
+{
+}
+
+WeakMapImpl::WeakMapImpl(JSContextGroupRef group)
+{
+    m_context = JSGlobalContextCreateInGroup(group, 0);
+    // Deleted by GC when m_context's globalObject gets collected.
+    m_map = JSWeakObjectMapCreate(m_context, 0, unusedWeakObjectMapCallback);
+}
+
+WeakMapImpl::~WeakMapImpl()
+{
+    JSGlobalContextRelease(m_context);
+    m_context = 0;
+    m_map = 0;
+}
+
+typedef HashMap<JSContextGroupRef, RefPtr<WeakMapImpl> > WeakMapSet;
+static WeakMapSet weakMaps;
+
+WeakMap::~WeakMap()
+{
+    // If this is the last WeakMap instance left, then we should remove
+    // the cached WeakMapImpl from the global weakMaps, too.
+    if (m_impl && m_impl->refCount() == 2) {
+        weakMaps.remove(JSContextGetGroup(m_impl->m_context));
+        ASSERT(m_impl->hasOneRef());
+    }
+}
+
+void WeakMap::set(JSContextRef context, void *key, JSObjectRef object)
+{
+    if (!m_impl) {
+        JSContextGroupRef group = JSContextGetGroup(context);
+        WeakMapSet::AddResult entry = weakMaps.add(group, 0);
+        if (entry.isNewEntry)
+            entry.iterator->second = adoptRef(new WeakMapImpl(group));
+        m_impl = entry.iterator->second;
+    }
+    JSWeakObjectMapSet(m_impl->m_context, m_impl->m_map, key, object);
+}
+
+JSObjectRef WeakMap::get(void* key)
+{
+    if (!m_impl)
+        return 0;
+    return JSWeakObjectMapGet(m_impl->m_context, m_impl->m_map, key);
+}
+
+void WeakMap::remove(void *key)
+{
+    if (!m_impl)
+        return;
+    JSWeakObjectMapRemove(m_impl->m_context, m_impl->m_map, key);
+}
+
 // Cache QtInstances
 typedef QMultiHash<void*, QtInstance*> QObjectInstanceMap;
 static QObjectInstanceMap cachedInstances;
index 54df34b..e5d2a93 100644 (file)
@@ -109,6 +109,7 @@ private:
     QPointer<QObject> m_object;
     QObject* m_hashkey;
     mutable QHash<QByteArray, QtRuntimeMethod*> m_methods;
+    WeakMap m_cachedMethods;
     mutable QHash<QString, QtField*> m_fields;
     ValueOwnership m_ownership;
 };
index 6a41e2c..9cf9c87 100644 (file)
@@ -1310,8 +1310,8 @@ static int findSignalIndex(const QMetaObject* meta, int initialIndex, QByteArray
 static JSClassRef prototypeForSignalsAndSlots()
 {
     static JSClassDefinition classDef = {
-        0, kJSClassAttributeNoAutomaticPrototype, 0, 0, 0, 0,
-        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
+        0, 0, 0, 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, QtRuntimeMethod::call, 0, 0, 0
     };
     static JSClassRef cls = JSClassCreate(&classDef);
     return cls;
@@ -1328,11 +1328,14 @@ QtRuntimeMethod::QtRuntimeMethod(JSContextRef ctx, QObject* object, const QByteA
 
 QtRuntimeMethod::~QtRuntimeMethod()
 {
+    if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
+        JSObjectSetPrivate(cachedWrapper, 0);
+    m_instance->m_cachedMethods.remove(this);
 }
 
 JSValueRef QtRuntimeMethod::call(JSContextRef context, JSObjectRef function, JSObjectRef /*thisObject*/, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
 {
-    QtRuntimeMethod* d = toRuntimeMethod(context, function);
+    QtRuntimeMethod* d = reinterpret_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
     if (!d) {
         setException(context, exception, QStringLiteral("cannot call function of deleted runtime method"));
         return JSValueMakeUndefined(context);
@@ -1375,57 +1378,53 @@ JSValueRef QtRuntimeMethod::disconnect(JSContextRef context, JSObjectRef functio
 
 JSObjectRef QtRuntimeMethod::jsObjectRef(JSContextRef context, JSValueRef* exception)
 {
-    if (m_jsObject)
-        return toRef(m_jsObject.get());
+    if (JSObjectRef cachedWrapper = m_instance->m_cachedMethods.get(this))
+        return cachedWrapper;
 
-    static JSStringRef connectStr = JSStringCreateWithUTF8CString("connect");
-    static JSStringRef disconnectStr = JSStringCreateWithUTF8CString("disconnect");
-    JSRetainPtr<JSStringRef> actualNameStr(Adopt, JSStringCreateWithUTF8CString(m_identifier.constData()));
-
-    JSObjectRef object = JSObjectMakeFunctionWithCallback(context, actualNameStr.get(), call);
-    JSValueProtect(context, object);
+    static const JSClassDefinition classDefForConnect = {
+        0, 0, "connect", 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, connect, 0, 0, 0
+    };
 
-    JSObjectRef generalFunctionProto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
-    JSObjectRef runtimeMethodProto = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
-    JSObjectSetPrototype(context, runtimeMethodProto, generalFunctionProto);
+    static const JSClassDefinition classDefForDisconnect = {
+        0, 0, "disconnect", 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, disconnect, 0, 0, 0
+    };
 
-    JSObjectSetPrototype(context, object, runtimeMethodProto);
+    static JSClassRef classRefConnect = JSClassCreate(&classDefForConnect);
+    static JSClassRef classRefDisconnect = JSClassCreate(&classDefForDisconnect);
+    bool isSignal = m_flags & MethodIsSignal;
+    JSObjectRef object = JSObjectMake(context, prototypeForSignalsAndSlots(), this);
+    JSObjectRef connectFunction = JSObjectMake(context, classRefConnect, this);
+    JSObjectRef disconnectFunction = JSObjectMake(context, classRefDisconnect, this);
+    JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
 
-    JSObjectRef connectFunction = JSObjectMakeFunctionWithCallback(context, connectStr, connect);
-    JSObjectSetPrototype(context, connectFunction, runtimeMethodProto);
+    static JSStringRef connectStr = JSStringCreateWithUTF8CString("connect");
+    static JSStringRef disconnectStr = JSStringCreateWithUTF8CString("disconnect");
+    static JSStringRef lengthStr = JSStringCreateWithUTF8CString("length");
+    static JSStringRef nameStr = JSStringCreateWithUTF8CString("name");
+    JSRetainPtr<JSStringRef> actualNameStr(Adopt, JSStringCreateWithUTF8CString(m_identifier.constData()));
 
-    JSObjectRef disconnectFunction = JSObjectMakeFunctionWithCallback(context, disconnectStr, disconnect);
-    JSObjectSetPrototype(context, disconnectFunction, runtimeMethodProto);
+    JSObjectSetProperty(context, connectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
+    JSObjectSetProperty(context, connectFunction, nameStr, JSValueMakeString(context, connectStr), attributes, exception);
+    JSObjectSetProperty(context, disconnectFunction, lengthStr, JSValueMakeNumber(context, isSignal ? 1 : 0), attributes, exception);
+    JSObjectSetProperty(context, disconnectFunction, nameStr, JSValueMakeString(context, disconnectStr), attributes, exception);
 
-    const JSPropertyAttributes attributes = kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete;
     JSObjectSetProperty(context, object, connectStr, connectFunction, attributes, exception);
     JSObjectSetProperty(context, object, disconnectStr, disconnectFunction, attributes, exception);
+    JSObjectSetProperty(context, object, lengthStr, JSValueMakeNumber(context, 0), attributes, exception);
+    JSObjectSetProperty(context, object, nameStr, JSValueMakeString(context, actualNameStr.get()), attributes, exception);
 
-    m_jsObject = PassWeak<JSObject>(toJS(object));
+    m_instance->m_cachedMethods.set(context, this, object);
 
     return object;
 }
 
-QtRuntimeMethod* QtRuntimeMethod::toRuntimeMethod(JSContextRef context, JSObjectRef object)
-{
-    JSObjectRef proto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0);
-    if (!proto)
-        return 0;
-    if (!JSValueIsObjectOfClass(context, proto, prototypeForSignalsAndSlots()))
-        return 0;
-    return static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(proto));
-}
-
 JSValueRef QtRuntimeMethod::connectOrDisconnect(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect)
 {
-    QtRuntimeMethod* d = toRuntimeMethod(context, thisObject);
+    QtRuntimeMethod* d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(thisObject));
     if (!d)
-        d = toRuntimeMethod(context, function);
-    if (!d) {
-        QString errorStr = QStringLiteral("QtMetaMethod.%1: Cannot connect to/from deleted QObject").arg(connect ?  QStringLiteral("connect") : QStringLiteral("disconnect"));
-        setException(context, exception, errorStr);
-        return JSValueMakeUndefined(context);
-    }
+        d = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(function));
 
     QString functionName = connect ? QStringLiteral("connect") : QStringLiteral("disconnect");
 
@@ -1461,9 +1460,11 @@ JSValueRef QtRuntimeMethod::connectOrDisconnect(JSContextRef context, JSObjectRe
 
         // object.signal.connect(someFunction);
         if (JSObjectIsFunction(context, targetFunction)) {
-            // object.signal.connect(otherObject.slot);
-            if (QtRuntimeMethod* targetMethod = toRuntimeMethod(context, targetFunction))
-                targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
+            if (JSValueIsObjectOfClass(context, targetFunction, prototypeForSignalsAndSlots())) {
+                // object.signal.connect(otherObject.slot);
+                if (QtRuntimeMethod* targetMethod = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(targetFunction)))
+                    targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context)));
+            }
         } else
             targetFunction = 0;
     } else {
index 6f7543d..ed0f4cf 100644 (file)
@@ -114,14 +114,14 @@ public:
     const QByteArray& name() { return m_identifier; }
 
 private:
-    static QtRuntimeMethod* toRuntimeMethod(JSContextRef, JSObjectRef);
+    static const JSStaticFunction connectFunction;
+    static const JSStaticFunction disconnectFunction;
 
     static JSValueRef connectOrDisconnect(JSContextRef ctx, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception, bool connect);
     QPointer<QObject> m_object;
     QByteArray m_identifier;
     int m_index;
     int m_flags;
-    Weak<JSObject> m_jsObject;
     QtInstance* m_instance;
 };
 
index 85e4775..8c13066 100644 (file)
@@ -1,3 +1,17 @@
+2012-08-22  Sheriff Bot  <webkit.review.bot@gmail.com>
+
+        Unreviewed, rolling out r126287.
+        http://trac.webkit.org/changeset/126287
+        https://bugs.webkit.org/show_bug.cgi?id=94708
+
+        It made WK1 layout testing 3.7x slower (>1hours) (Requested by
+        ossy on #webkit).
+
+        * tests/qobjectbridge/tst_qobjectbridge.cpp:
+        (tst_QObjectBridge::objectDeleted):
+        (tst_QObjectBridge::introspectQtMethods_data):
+        (tst_QObjectBridge::introspectQtMethods):
+
 2012-08-22  Allan Sandfeld Jensen  <allan.jensen@nokia.com>
 
         [Qt] Optionally support smooth-scrolling on all platforms
index 13d98af..2ce791d 100644 (file)
@@ -1879,7 +1879,7 @@ void tst_QObjectBridge::objectDeleted()
     evalJS("bar.intProperty = 123;");
     QCOMPARE(qobj->intProperty(), 123);
     qobj->resetQtFunctionInvoked();
-    evalJS("bar.myInvokable.call(bar);");
+    evalJS("bar.myInvokable(bar);");
     QCOMPARE(qobj->qtFunctionInvoked(), 0);
 
     // do this, to ensure that we cache that it implements call
@@ -2148,15 +2148,15 @@ void tst_QObjectBridge::introspectQtMethods_data()
     QTest::addColumn<QStringList>("expectedPropertyNames");
 
     QTest::newRow("myObject.mySignal")
-        << "myObject" << "mySignal" << (QStringList() << "connect" << "disconnect" << "name");
+        << "myObject" << "mySignal" << (QStringList() << "connect" << "disconnect" << "length" << "name");
     QTest::newRow("myObject.mySlot")
-        << "myObject" << "mySlot" << (QStringList() << "connect" << "disconnect" << "name");
+        << "myObject" << "mySlot" << (QStringList() << "connect" << "disconnect" << "length" << "name");
     QTest::newRow("myObject.myInvokable")
-        << "myObject" << "myInvokable" << (QStringList() << "connect" << "disconnect" << "name");
+        << "myObject" << "myInvokable" << (QStringList() << "connect" << "disconnect" << "length" << "name");
     QTest::newRow("myObject.mySignal.connect")
-        << "myObject.mySignal" << "connect" << (QStringList() << "name");
+        << "myObject.mySignal" << "connect" << (QStringList() << "length" << "name");
     QTest::newRow("myObject.mySignal.disconnect")
-        << "myObject.mySignal" << "disconnect" << (QStringList() << "name");
+        << "myObject.mySignal" << "disconnect" << (QStringList() << "length" << "name");
 }
 
 void tst_QObjectBridge::introspectQtMethods()
@@ -2177,7 +2177,7 @@ void tst_QObjectBridge::introspectQtMethods()
         QCOMPARE(evalJS("descriptor.set"), sUndefined);
         QCOMPARE(evalJS(QString::fromLatin1("descriptor.value === %0['%1']").arg(methodLookup).arg(name)), sTrue);
         QCOMPARE(evalJS(QString::fromLatin1("descriptor.enumerable")), sFalse);
-        QCOMPARE(evalJS(QString::fromLatin1("descriptor.configurable")), sFalse);
+        QCOMPARE(evalJS(QString::fromLatin1("descriptor.configurable")), sTrue);
     }
 
     QVERIFY(evalJSV("var props=[]; for (var p in myObject.deleteLater) {props.push(p);}; props.sort()").toStringList().isEmpty());