Bug 192943 – Moving non-critical initializations to a parallel thread can speed up process launch time by 15%.

• Component: WebKit2
• Status: NEW

[Suresh Koppisetty]

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

[Suresh Koppisetty]

Created attachment 357845 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Suresh Koppisetty]

Created attachment 357860 [details]
Patch

[Suresh Koppisetty]

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.

[Chris Dumez]

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

[Sam Weinig]

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.

[Suresh Koppisetty]

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.

[Suresh Koppisetty]

Created attachment 357870 [details]
Patch

[Suresh Koppisetty]

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

[Suresh Koppisetty]

Created attachment 357902 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Alex Christensen]

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.