Bug 93897 – [Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html fails

• Component: Platform
• Status: RESOLVED FIXED

[Simon Hausmann]

The test fails because it relies on the name of runtime methods to be set properly. r125428 regressed this.

This might require re-introducing the per-method JSClassRef unless setting the name property directly works. I'm working on a fix for this, so skipping the test for now.

[Simon Hausmann]

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

instead of [Callback object] :)

[Caio Marcelo de Oliveira Filho]

(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.

[Simon Hausmann]

(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.

[Simon Hausmann]

Created attachment 158309 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Simon Hausmann]

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).

[Simon Hausmann]

Caio, any thoughts? :)

[Caio Marcelo de Oliveira Filho]

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.

[Simon Hausmann]

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!

[Simon Hausmann]

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.

[Simon Hausmann]

Created attachment 159066 [details]
Patch

[Simon Hausmann]

Created attachment 159068 [details]
Patch

Updated patch without debug output

[Caio Marcelo de Oliveira Filho]

Comment on attachment 159068 [details]
Patch

LGTM!

[Simon Hausmann]

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

[Csaba Osztrogonác]

(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*)

[WebKit Review Bot]

Re-opened since this is blocked by 94606

[Simon Hausmann]

Created attachment 159884 [details]
Patch

Updated patch that re-introduces internal Weak<> API usage until the rest of the bindings are re-written

[Simon Hausmann]

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

[Csaba Osztrogonác]

(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?

[Csaba Osztrogonác]

(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. :)

[WebKit Review Bot]

Re-opened since this is blocked by 94708

[Csaba Osztrogonác]

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)

[Csaba Osztrogonác]

+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.

[Simon Hausmann]

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.

[Simon Hausmann]

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).

[Simon Hausmann]

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