Moving non-critical initializations to a parallel thread can speed up process launch time by 15%. <rdar://problem/46877677>. _RegisterApplication, _LSSetApplicationInformationItem and _accessibilityInitialize are non-critical work and they can be done in a parallel thread to speed up process launch time. Following times are measured from artraces captured under different scenarios. See <rdar://problem/46877677>. System Config: ————————————— MacBook Pro (13-inch, Late 2013 model running 18E158) Clean system: ————————————— Mean total process launch time: 166.56 ms Mean Process Launch time: 58.88 ms Mean Init New Web Process time: 74.64 ms Root with a clean trunk: ————————————— Safari : 32656d6f Webkit: r239297 Mean total process launch time: 176.53 ms Mean Process Launch time: 62.72 ms Mean Init New Web Process time: 83.6 ms Root with proposed changes on top of trunk: ————————————— Safari : 32656d6f Webkit: r239297 Mean total process launch time: 150.12 ms Mean Process Launch time: 64.19 ms Mean Init New Web Process time: 74.18 ms Perf Improvement: ————————————— 176.53 - 150.12 = 26.41 ms (~15%)
Created attachment 357845 [details] Patch
Comment on attachment 357845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357845&action=review > Source/WebKit/WebProcess/WebProcess.h:471 > + dispatch_queue_t m_internalQueue; Could we use a WTF::WorkQueue? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:421 > + dispatch_release(m_internalQueue); Then we wouldn't need this.
Comment on attachment 357845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357845&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:368 > + dispatch_async(m_internalQueue, ^{ WebProcess::updateProcessName() would have to use the same (serial) queue, otherwise, the call to _RegisterApplication() may happen *after* we've tried to set the process name and setting the process name would fail.
Comment on attachment 357845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357845&action=review >> Source/WebKit/WebProcess/WebProcess.h:471 >> + dispatch_queue_t m_internalQueue; > > Could we use a WTF::WorkQueue? You could also use OSObjectPtr<dispatch_queue_t>. We just like to avoid explicit memory management in WebKit.
Created attachment 357860 [details] Patch
Comment on attachment 357845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357845&action=review >>> Source/WebKit/WebProcess/WebProcess.h:471 >>> + dispatch_queue_t m_internalQueue; >> >> Could we use a WTF::WorkQueue? > > You could also use OSObjectPtr<dispatch_queue_t>. We just like to avoid explicit memory management in WebKit. Using OSObjectPtr<dispatch_queue_t>. Thanks for the suggestions. >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:368 >> + dispatch_async(m_internalQueue, ^{ > > WebProcess::updateProcessName() would have to use the same (serial) queue, otherwise, the call to _RegisterApplication() may happen *after* we've tried to set the process name and setting the process name would fail. Righ, all of them happen in m_internalQueue serial queue. Changing comments a bit to convey the same message. >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:421 >> + dispatch_release(m_internalQueue); > > Then we wouldn't need this. Yes.
Comment on attachment 357860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357860&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:368 > + m_internalQueue = dispatch_queue_create([NSString stringWithFormat:@"com.apple.WebKit.WebProcessCocoa"].UTF8String, DISPATCH_QUEUE_SERIAL); I believe you should adoptOSObject(), otherwise you increment the ref count again and this leaks. Why aren't we setting a BACKGROUND QoS class like my code was doing? dispatch_queue_attr_t attr = DISPATCH_QUEUE_SERIAL; attr = dispatch_queue_attr_make_with_qos_class(attr, QOS_CLASS_BACKGROUND, 0); m_internalQueue = dispatch_queue_create("com.apple.WebKit.WebProcessCocoa", attr); Note that if you used WTF::WorkQueue, you'd simply pass the QoS when constructing it. I also do not understand the purpose of [NSString stringWithFormat] in your code, you're not formatting anything and you do not need a NSString*.
Comment on attachment 357860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357860&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:190 > + [NSApplication _accessibilityInitialize]; Have you verified with the AppKit team that this is safe to call from a non-main thread? Most things in AppKit are not.
Comment on attachment 357860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357860&action=review >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:368 >> + m_internalQueue = dispatch_queue_create([NSString stringWithFormat:@"com.apple.WebKit.WebProcessCocoa"].UTF8String, DISPATCH_QUEUE_SERIAL); > > I believe you should adoptOSObject(), otherwise you increment the ref count again and this leaks. > > Why aren't we setting a BACKGROUND QoS class like my code was doing? > > dispatch_queue_attr_t attr = DISPATCH_QUEUE_SERIAL; > attr = dispatch_queue_attr_make_with_qos_class(attr, QOS_CLASS_BACKGROUND, 0); > m_internalQueue = dispatch_queue_create("com.apple.WebKit.WebProcessCocoa", attr); > > Note that if you used WTF::WorkQueue, you'd simply pass the QoS when constructing it. > > I also do not understand the purpose of [NSString stringWithFormat] in your code, you're not formatting anything and you do not need a NSString*. My bad. I don't need [NSString stringWithFormat]. I was only trying to get all of them in a single queue till now. I still haven't yet tried out by changing QOS classes. Getting a root to verify this.
Created attachment 357870 [details] Patch
(In reply to Sam Weinig from comment #8) > Comment on attachment 357860 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357860&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:190 > > + [NSApplication _accessibilityInitialize]; > > Have you verified with the AppKit team that this is safe to call from a > non-main thread? Most things in AppKit are not. Checking with them.
Created attachment 357902 [details] Patch
Comment on attachment 357902 [details] Patch Attachment 357902 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10501139 Number of test failures exceeded the failure limit.
Created attachment 357915 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 357902 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.