WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
93897
[Qt] REGRESSION(
r125428
): fast/profiler/nested-start-and-stop-profiler.html fails
https://bugs.webkit.org/show_bug.cgi?id=93897
Summary
[Qt] REGRESSION(r125428): fast/profiler/nested-start-and-stop-profiler.html f...
Simon Hausmann
Reported
2012-08-13 13:59:29 PDT
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.
Attachments
Patch
(14.99 KB, patch)
2012-08-14 06:00 PDT
,
Simon Hausmann
no flags
Details
Formatted Diff
Diff
Patch
(15.65 KB, patch)
2012-08-17 02:59 PDT
,
Simon Hausmann
no flags
Details
Formatted Diff
Diff
Patch
(15.53 KB, patch)
2012-08-17 03:13 PDT
,
Simon Hausmann
no flags
Details
Formatted Diff
Diff
Patch
(19.68 KB, patch)
2012-08-22 02:06 PDT
,
Simon Hausmann
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Simon Hausmann
Comment 1
2012-08-14 03:29:08 PDT
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
Comment 2
2012-08-14 03:58:26 PDT
(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
Comment 3
2012-08-14 04:29:56 PDT
(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
Comment 4
2012-08-14 06:00:41 PDT
Created
attachment 158309
[details]
Patch
Kenneth Rohde Christiansen
Comment 5
2012-08-14 06:09:51 PDT
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 6
2012-08-14 06:18:31 PDT
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
Comment 7
2012-08-16 00:33:43 PDT
Caio, any thoughts? :)
Caio Marcelo de Oliveira Filho
Comment 8
2012-08-16 08:22:40 PDT
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 9
2012-08-17 02:51:56 PDT
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 10
2012-08-17 02:52:51 PDT
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
Comment 11
2012-08-17 02:59:39 PDT
Created
attachment 159066
[details]
Patch
Simon Hausmann
Comment 12
2012-08-17 03:13:10 PDT
Created
attachment 159068
[details]
Patch Updated patch without debug output
Caio Marcelo de Oliveira Filho
Comment 13
2012-08-20 05:26:27 PDT
Comment on
attachment 159068
[details]
Patch LGTM!
Simon Hausmann
Comment 14
2012-08-21 03:52:46 PDT
Committed
r126146
: <
http://trac.webkit.org/changeset/126146
>
Csaba Osztrogonác
Comment 15
2012-08-21 09:01:53 PDT
(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
Comment 16
2012-08-21 09:14:24 PDT
Re-opened since this is blocked by 94606
Simon Hausmann
Comment 17
2012-08-22 02:06:11 PDT
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
Comment 18
2012-08-22 03:41:59 PDT
Committed
r126287
: <
http://trac.webkit.org/changeset/126287
>
Csaba Osztrogonác
Comment 19
2012-08-22 06:54:09 PDT
(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
Comment 20
2012-08-22 06:55:34 PDT
(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
Comment 21
2012-08-22 07:09:24 PDT
Re-opened since this is blocked by 94708
Csaba Osztrogonác
Comment 22
2012-08-23 03:24:31 PDT
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
Comment 23
2012-08-23 03:26:56 PDT
+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
Comment 24
2012-08-23 06:00:35 PDT
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 25
2012-08-29 02:47:33 PDT
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
Comment 26
2012-08-29 03:15:45 PDT
Committed
r126976
: <
http://trac.webkit.org/changeset/126976
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug