Bug 93897

This also affects numerous unit tests that do someQObject.someMethod.toString() and expect function someMethod() { [ native code ] } instead of [Callback object] :)

(In reply to comment #1) > This also affects numerous unit tests that do someQObject.someMethod.toString() and expect function someMethod() { > [ native code ] > } > > instead of [Callback object] :) You mean our Qt API tests? If so, regardless the fix here, IMHO those API tests are "over-detailed", I don't think people are/should be relying on the string representation of these methods. I remember Noam pointing this out recently too.

(In reply to comment #2) > (In reply to comment #1) > > This also affects numerous unit tests that do someQObject.someMethod.toString() and expect function someMethod() { > > [ native code ] > > } > > > > instead of [Callback object] :) > > You mean our Qt API tests? If so, regardless the fix here, IMHO those API tests are "over-detailed", I don't think people are/should be relying on the string representation of these methods. I remember Noam pointing this out recently too. I agree about that. But the fix will automatically fix this, too :) I'm working on a change to make the functions actual JS functions again instead of JSClass instances with a callAsFunction callback set.

Created attachment 158309 [details] Patch

Comment on attachment 158309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158309&action=review > Source/WebCore/ChangeLog:21 > + The length property on a function signifies the number of arguments, but in all three cases that number is which three cases? > Source/WebCore/ChangeLog:22 > + actually variable, because of overloading. so as it is variable you just dont expose it? I think this part of the changelog could be more clear > Source/WebCore/bridge/qt/qt_runtime.cpp:1384 > + JSObjectRef proto = JSValueToObject(context, JSObjectGetPrototype(context, object), 0); > + if (!proto) > + return 0; > + if (!JSValueIsObjectOfClass(context, proto, prototypeForSignalsAndSlots())) > + return 0; Are these valid cases? or should you add asserts as well?

Comment on attachment 158309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158309&action=review >> Source/WebCore/ChangeLog:21 >> + The length property on a function signifies the number of arguments, but in all three cases that number is > > which three cases? run-time method itself, connect and disconnect. All three are JS objects that had a length property. >> Source/WebCore/ChangeLog:22 >> + actually variable, because of overloading. > > so as it is variable you just dont expose it? > > I think this part of the changelog could be more clear I can make a separate paragraph explaining this change of behaviour (that I also briefly discussed with Kent). >> Source/WebCore/bridge/qt/qt_runtime.cpp:1384 >> + return 0; > > Are these valid cases? or should you add asserts as well? They are valid cases, people can do stuff like this: myQObject = ... connectFunction = myQObject.mySignal.disconnect; myObject = new Object; myObject.connect = connectFunction; myObject.connect(someOtherStuff); This should not assert but gracefully fail at run-time. The caller of toRuntimeMethod will throw an exception in fact (in JS).

Caio, any thoughts? :)

Comment on attachment 158309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158309&action=review The approach looks good to me. > Source/WebCore/ChangeLog:16 > + This patch changes run-time methods back to being real function objects that have a correct name and > + have Function.prototype in their prototype change. Injected right before the Function.prototype prototype > + is however our own prototype, which uses private data (JSObjectGetPrivate) to store a pointer to our > + C++ QtRuntimeMethod object. This complicates the retrieval of the pointer to that instance slightly, which I think some explanation why we need one proto object per function would be nice. From my understanding the main motivation is that you can't use JSObject{Set,Get}Private with real function objects. > Source/WebCore/bridge/qt/qt_runtime.cpp:1285 > + static JSClassRef cls = JSClassCreate(&kJSClassDefinitionEmpty); Minor nit: you could use kJSClassAttributeNoAutomaticPrototype inside the class definition since we'll always set the prototype for objects like this. But this is really minor.

Comment on attachment 158309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158309&action=review >> Source/WebCore/ChangeLog:16 >> + C++ QtRuntimeMethod object. This complicates the retrieval of the pointer to that instance slightly, which > > I think some explanation why we need one proto object per function would be nice. From my understanding the main motivation is that you can't use JSObject{Set,Get}Private with real function objects. That's right. Ok, I'll reword this :) >> Source/WebCore/bridge/qt/qt_runtime.cpp:1285 >> + static JSClassRef cls = JSClassCreate(&kJSClassDefinitionEmpty); > > Minor nit: you could use kJSClassAttributeNoAutomaticPrototype inside the class definition since we'll always set the prototype for objects like this. But this is really minor. Good idea!

Comment on attachment 158309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158309&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:1437 > if (JSValueIsObjectOfClass(context, targetFunction, prototypeForSignalsAndSlots())) { > // object.signal.connect(otherObject.slot); > - if (QtRuntimeMethod* targetMethod = static_cast<QtRuntimeMethod*>(JSObjectGetPrivate(targetFunction))) > + if (QtRuntimeMethod* targetMethod = toRuntimeMethod(context, targetFunction)) > targetObject = toRef(QtInstance::getQtInstance(targetMethod->m_object.data(), d->m_instance->rootObject(), QtInstance::QtOwnership)->createRuntimeObject(toJS(context))); > } As it turns out, the two nested if()'s can be combined into one, since toRuntimeMethod() does the same JSValueIsObjectOfClass check (on the prototype). I'll do that in the next version of the patch.

Created attachment 159066 [details] Patch

Created attachment 159068 [details] Patch Updated patch without debug output

Comment on attachment 159068 [details] Patch LGTM!

Committed r126146: <http://trac.webkit.org/changeset/126146>

(In reply to comment #14) > Committed r126146: <http://trac.webkit.org/changeset/126146> It made all tests assert in debug mode: STDERR: ASSERTION FAILED: obj->inherits(&JSCallbackObject<JSGlobalObject>::s_info) || obj->inherits(&JSCallbackObject<JSNonFinalObject>::s_info) STDERR: /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/JavaScriptCore/API/JSWeakObjectMapRefPrivate.cpp(60) : void JSWeakObjectMapSet(const OpaqueJSContext*, OpaqueJSWeakObjectMap*, void*, OpaqueJSValue*)

Re-opened since this is blocked by 94606

Created attachment 159884 [details] Patch Updated patch that re-introduces internal Weak<> API usage until the rest of the bindings are re-written

Committed r126287: <http://trac.webkit.org/changeset/126287>

(In reply to comment #18) > Committed r126287: <http://trac.webkit.org/changeset/126287> Unfortunately we have problems with this patch again. :( It made WK1 layout testing time 5x slower r126286 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/41584 - ( 18 mins, 8 secs ) r126287 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/41582 - ( 1 hrs, 8 mins, 5 secs ) Have you got any idea what this problem is? (WK2 works fine without performance regression) Have you got a quick fix or should we roll it out again?

(In reply to comment #19) > (In reply to comment #18) > > Committed r126287: <http://trac.webkit.org/changeset/126287> > It made WK1 layout testing time 5x slower I meant 3.8x slower. :)

Re-opened since this is blocked by 94708

I experimented with this slowdown a little bit, and I got the following results: r126286 ------- $ Tools/Scripts/run-webkit-tests 21m20.883s $ Tools/Scripts/run-webkit-tests http 3m50.175s $ Tools/Scripts/run-webkit-tests --no-http 17m1.279s r126287 ------- $ Tools/Scripts/run-webkit-tests > 1 hour (I stopped after 15000 tests and 1 hour runtime) $ Tools/Scripts/run-webkit-tests http 5m34.990s $ Tools/Scripts/run-webkit-tests --no-http 24m49.050s The runtime without http tests become "only" 1.5x, but with http tests is non-usable sloooow. It seems something wrong happens during running https tests ... It can explain why we can't see this slowdown with parallel NRWT. (But unfortunately we can't run parallel NRWT on the bots until https://bugs.webkit.org/show_bug.cgi?id=77730 is fixed)

+info for slowndown: The original http://trac.webkit.org/changeset/126146 didn't cause slowdown in release mode, "only" the zillion assertion in debug mode. But the new http://trac.webkit.org/changeset/126287 fixed the assertions, but caused this slowdown in release and debug mode too.

Ok, I can reproduce the slowdown on my machine, for http tests the slowdown is about ~1 minute (difference), a little here and there. I'll take a look and see if I can find the culprit(s) in a profile. I'd really like to see the patch go in again soon, because while it unskips only one layout tests, it actually fixes a whole bunch of unit tests that are currently failing.

Comment on attachment 159884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159884&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:1358 > + JSValueProtect(context, object); This stray JSValueProtect call caused the methods to always remain on the heap and slowly accumulate. That would also explain why it a) require a bigger set of tests to actually notice the regression and b) the regression is more visibile on the bot than on my machine (different amounts of memory available) I'll go and land the change again without this call (that is obviously buggy because it's lacking a JSValueUnprotect but not actually needed in the first place).

Committed r126976: <http://trac.webkit.org/changeset/126976>