Bug 192943

Summary: Moving non-critical initializations to a parallel thread can speed up process launch time by 15%.
Product: WebKit Reporter: Suresh Koppisetty <skoppisetty>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: cdumez, ews, ggaren, rniwa, sam, skoppisetty
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Macintosh   
OS: macOS 10.14   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
skoppisetty: review?, ews: commit-queue-
Archive of layout-test-results from ews106 for mac-sierra-wk2 none

Description Suresh Koppisetty 2018-12-20 11:51:26 PST
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%)
Comment 1 Suresh Koppisetty 2018-12-20 11:52:48 PST
Created attachment 357845 [details]
Patch
Comment 2 Chris Dumez 2018-12-20 11:57:31 PST
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 3 Chris Dumez 2018-12-20 12:02:07 PST
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 4 Chris Dumez 2018-12-20 12:05:47 PST
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.
Comment 5 Suresh Koppisetty 2018-12-20 13:38:53 PST
Created attachment 357860 [details]
Patch
Comment 6 Suresh Koppisetty 2018-12-20 13:39:37 PST
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 7 Chris Dumez 2018-12-20 13:47:40 PST
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 8 Sam Weinig 2018-12-20 14:17:36 PST
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 9 Suresh Koppisetty 2018-12-20 14:18:59 PST
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.
Comment 10 Suresh Koppisetty 2018-12-20 14:21:12 PST
Created attachment 357870 [details]
Patch
Comment 11 Suresh Koppisetty 2018-12-20 14:35:48 PST
(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.
Comment 12 Suresh Koppisetty 2018-12-20 16:59:30 PST
Created attachment 357902 [details]
Patch
Comment 13 Build Bot 2018-12-20 18:50:47 PST
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.
Comment 14 Build Bot 2018-12-20 18:50:49 PST
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