Summary: | [WTR] Move text output accumulation to the UIProcess | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||
Component: | Tools / Tests | Assignee: | Martin Robinson <mrobinson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, sam, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 103653 | ||||||||||
Attachments: |
|
Description
Martin Robinson
2012-12-05 22:11:33 PST
Created attachment 178360 [details]
Patch
This patch is large, but generally mechanical. 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? (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. Created attachment 178429 [details]
Patch
Comment on attachment 178429 [details] Patch Attachment 178429 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15212979 Created attachment 178439 [details]
Fix bad merge
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! Comment on attachment 178439 [details] Fix bad merge Clearing flags on attachment: 178439 Committed r137127: <http://trac.webkit.org/changeset/137127> All reviewed patches have been landed. Closing bug. 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 (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 |