Bug 104214 – [WTR] Move text output accumulation to the UIProcess

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Martin Robinson]

Instead of accumulating text output in the InjectedBundle and then shipping it to the UIProcess once a test is finished, WTR should immediately send any text output to the UIProcess. This will allow WTR to output text from the UIProcess as well, which is immediately useful to test authentication in WebKit2.

[Martin Robinson]

Created attachment 178360 [details]
Patch

[Martin Robinson]

This patch is large, but generally mechanical.

[Darin Adler]

Comment on attachment 178360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178360&action=review

What effect did this have on the speed of test running? Slower? Faster? No noticeable effect?

Looks like the patch did not apply.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:220
> +        outputText(String::format("Boolean value for key \"%s\" not found in dictionary\n", key));

More efficient to use makeString instead of String::format.

    outputText(makeString("Boolean value for key \", key, \" not found in dictionary\n"));

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:40
> +#include <wtf/text/WTFString.h>

Normally wtf/Forward.h would be sufficient in a header like this one, since there’s no actual use of WTF::String, just a declaration of an argument of const String& type.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:70
> +    void dumpBackForwardListsForAllPages(WTF::StringBuilder&);

Should not need the WTF:: prefix. StringBuilder.h should be taking care of that.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:84
> +    void outputText(const WTF::String&);

Should not need the WTF:: prefix. WTFString.h should be taking care of that.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:64
> +using WTF::StringBuilder;

Should not need this. StringBuilder.h should do this.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:487
> +    stringBuilder.appendLiteral("\n");

Can use character append here.

> Tools/WebKitTestRunner/TestInvocation.cpp:109
> +    , m_textOutput(WTF::adoptPtr(new WTF::StringBuilder()))

Should not need these WTF:: prefixes, because WTF headers should do the using thing. No need for () after StringBuilder either. Looks like you moved this from elsewhere and it had the same problem.

> Tools/WebKitTestRunner/TestInvocation.h:72
> +    OwnPtr<WTF::StringBuilder> m_textOutput;

Why the OwnPtr? Why not just StringBuilder?

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 178360 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178360&action=review

Thanks for the review! I'll keep testing this a bit before landing.

> What effect did this have on the speed of test running? Slower? Faster? No noticeable effect?

I ran time run-webkit-tests --gtk -2 --no-show-results --no-retry-failures editing fast css2.1 css3 css1 dom, which is over 13000 tests.

Before the patch I saw:
real    10m25.461s
user    8m23.507s
sys 2m12.272s

with the patch I got:
real	10m46.092s
user	8m46.305s
sys	2m16.777s

So it looks like this may have a small performance penalty, though I'll investigate if this scales proportionally with the number of tests or is sampling error.

> Looks like the patch did not apply.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:220
> > +        outputText(String::format("Boolean value for key \"%s\" not found in dictionary\n", key));
> 
> More efficient to use makeString instead of String::format.

Okay.

>     outputText(makeString("Boolean value for key \", key, \" not found in dictionary\n"));
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:40
> > +#include <wtf/text/WTFString.h>
> 
> Normally wtf/Forward.h would be sufficient in a header like this one, since there’s no actual use of WTF::String, just a declaration of an argument of const String& type.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:70
> > +    void dumpBackForwardListsForAllPages(WTF::StringBuilder&);
> 
> Should not need the WTF:: prefix. StringBuilder.h should be taking care of that.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:84
> > +    void outputText(const WTF::String&);
> 
> Should not need the WTF:: prefix. WTFString.h should be taking care of that.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:64
> > +using WTF::StringBuilder;
> 
> Should not need this. StringBuilder.h should do this.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:487
> > +    stringBuilder.appendLiteral("\n");
> 
> Can use character append here.
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:109
> > +    , m_textOutput(WTF::adoptPtr(new WTF::StringBuilder()))
> 
> Should not need these WTF:: prefixes, because WTF headers should do the using thing. No need for () after StringBuilder either. Looks like you moved this from elsewhere and it had the same problem.
> 
> > Tools/WebKitTestRunner/TestInvocation.h:72
> > +    OwnPtr<WTF::StringBuilder> m_textOutput;
> 
> Why the OwnPtr? Why not just StringBuilder?

I copied this over from the InjectedBundle. In my new version of the patch I made it just StringBuilder and fixed all the other issues you mentioned.

[Martin Robinson]

Created attachment 178429 [details]
Patch

[EFL EWS Bot]

Comment on attachment 178429 [details]
Patch

Attachment 178429 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15212979

[Martin Robinson]

Created attachment 178439 [details]
Fix bad merge

[Martin Robinson]

I ran the editing tests multiple times with these results:

before:
1m43.296s
1m43.785s
1m47.829s
1m47.173s

after:
1m46.336s
1m48.317s
1m46.510s
1m45.438s

I'll land this tomorrow morning and watch the bots very closely. If I detect significant performance degradation on any of the WebKit2 platforms I'll roll it out and reevaluate. Thanks again!

[WebKit Review Bot]

Comment on attachment 178439 [details]
Fix bad merge

Clearing flags on attachment: 178439

Committed r137127: <http://trac.webkit.org/changeset/137127>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

LayoutTests are no longer running on WK2 EFL, it looks like it may be related to this patch:

03:03:15.572 490 worker/3 http/tests/inspector/network/async-xhr-json-mime-type.html crashed, (stderr lines):
03:03:15.572 490   SHOULD NEVER BE REACHED
03:03:15.572 490   /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/WebKitTestRunner/TestInvocation.cpp(559) : void WTR::TestInvocation::didReceiveMessageFromInjectedBundle(WKStringRef, WKTypeRef)
03:03:15.572 490   1   0x427ebc WTR::TestInvocation::didReceiveMessageFromInjectedBundle(OpaqueWKString const*, void const*)
03:03:15.572 490   2   0x41f43d WTR::TestController::didReceiveMessageFromInjectedBundle(OpaqueWKString const*, void const*)
03:03:15.572 490   3   0x41f143 WTR::TestController::didReceiveMessageFromInjectedBundle(OpaqueWKContext const*, OpaqueWKString const*, void const*, void const*)
03:03:15.572 490   4   0x7f91af49b675 WebKit::WebContextInjectedBundleClient::didReceiveMessageFromInjectedBundle(WebKit::WebContext*, WTF::String const&, WebKit::APIObject*)
03:03:15.572 490   5   0x7f91af48895a WebKit::WebContext::didReceiveMessageFromInjectedBundle(WTF::String const&, WebKit::APIObject*)
03:03:15.572 490   6   0x7f91af4894b2 WebKit::WebContext::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
03:03:15.572 490   7   0x7f91af3f6c7a CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
03:03:15.572 490   8   0x7f91af4892f5 WebKit::WebContext::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
03:03:15.572 490   9   0x7f91af50740a WebKit::WebProcessProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
03:03:15.572 490   10  0x7f91af3eb5d8 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)
03:03:15.572 490   11  0x7f91af3eb744 CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)
03:03:15.572 490   12  0x7f91af3eb98f CoreIPC::Connection::dispatchOneMessage()
03:03:15.572 490   13  0x7f91af3f6226 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
03:03:15.572 490   14  0x7f91af3f602c WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
03:03:15.572 490   15  0x7f91b6920a62 WTF::Function<void ()>::operator()() const
03:03:15.572 490   16  0x7f91b2c0180b WebCore::RunLoop::performWork()
03:03:15.572 490   17  0x7f91b367b51f WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
03:03:15.572 490   18  0x7f91ae7bb621
03:03:15.572 490   19  0x7f91ae7ba571
03:03:15.572 490   20  0x7f91ae7baab7 ecore_main_loop_begin
03:03:15.572 490   21  0x4343d5 WTR::TestController::platformRunUntil(bool&, double)
03:03:15.572 490   22  0x41f112 WTR::TestController::runUntil(bool&, WTR::TestController::TimeoutDuration)
03:03:15.572 490   23  0x4261dd WTR::TestInvocation::invoke()
03:03:15.572 490   24  0x41ee4a WTR::TestController::runTest(char const*)
03:03:15.572 490   25  0x41ef83 WTR::TestController::runTestingServerLoop()
03:03:15.572 490   26  0x41f01d WTR::TestController::run()
03:03:15.572 490   27  0x41c953 WTR::TestController::TestController(int, char const**)
03:03:15.572 490   28  0x43456e main
03:03:15.572 490   29  0x7f91ad31b76d __libc_start_main
03:03:15.572 490   30  0x41b279
03:03:15.572 490   LEAK: 31 XMLHttpRequest
03:03:15.572 490   LEAK: 162 CachedResource
03:03:15.572 490   LEAK: 4 Range
03:03:15.572 490   LEAK: 1482 WebCoreNode

[Martin Robinson]

(In reply to comment #11)
> LayoutTests are no longer running on WK2 EFL, it looks like it may be related to this patch:

Thanks for letting me know. I filed a bug here:  https://bugs.webkit.org/show_bug.cgi?id=104549