Bug 192943 - Moving non-critical initializations to a parallel thread can speed up process launch time by 15%.
Summary: Moving non-critical initializations to a parallel thread can speed up process...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh macOS 10.14
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-20 11:51 PST by Suresh Koppisetty
Modified: 2018-12-20 18:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.18 KB, patch)
2018-12-20 11:52 PST, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
Patch (5.34 KB, patch)
2018-12-20 13:38 PST, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2018-12-20 14:21 PST, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
Patch (5.42 KB, patch)
2018-12-20 16:59 PST, Suresh Koppisetty
skoppisetty: review?
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (1.11 MB, application/zip)
2018-12-20 18:50 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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