Bug 191160 - Don’t use the main queue to create an XPC connection
Summary: Don’t use the main queue to create an XPC connection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Mac macOS 10.14
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-01 11:32 PDT by Suresh Koppisetty
Modified: 2019-01-22 12:50 PST (History)
11 users (show)

See Also:


Attachments
Patch (6.04 KB, patch)
2018-11-01 11:59 PDT, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
artrace showing the distribution of total process launch time for a system which has a Safari root with no changes (deleted)
2018-11-01 12:15 PDT, Suresh Koppisetty
no flags Details
artrce screenshot showing total process launch time with a Safari root with proposed changes (deleted)
2018-11-01 12:16 PDT, Suresh Koppisetty
no flags Details
artrace showing the process launch time for a system which has a Safari root with no changes (deleted)
2018-11-01 12:19 PDT, Suresh Koppisetty
no flags Details
artrace showing the process launch time for a system which has a Safari root with proposed changes (deleted)
2018-11-01 12:20 PDT, Suresh Koppisetty
no flags Details
Patch (6.05 KB, patch)
2018-11-01 13:34 PDT, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.40 MB, application/zip)
2018-11-01 14:36 PDT, EWS Watchlist
no flags Details
Patch (5.92 KB, patch)
2018-11-01 16:09 PDT, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2018-11-05 10:55 PST, Suresh Koppisetty
no flags Details | Formatted Diff | Diff
Patch (3.30 KB, patch)
2018-11-06 14:16 PST, Suresh Koppisetty
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Suresh Koppisetty 2018-11-01 11:32:53 PDT
Don’t use the main queue to create an XPC connection as xpc_connection_set_bootstrap does a dispatch_mach_send_barrier_f on this queue which delays the sending of the subsequent bootstrap message
Comment 1 Suresh Koppisetty 2018-11-01 11:59:59 PDT
Created attachment 353626 [details]
Patch
Comment 2 Suresh Koppisetty 2018-11-01 12:15:01 PDT
Created attachment 353628 [details]
artrace showing the distribution of total process launch time for a system which has a Safari root with no changes
Comment 3 Suresh Koppisetty 2018-11-01 12:16:35 PDT
Created attachment 353630 [details]
artrce screenshot showing total process launch time with a Safari root with proposed changes
Comment 4 Suresh Koppisetty 2018-11-01 12:19:32 PDT
Created attachment 353633 [details]
artrace showing the process launch time for a system which has a Safari root with no changes
Comment 5 Suresh Koppisetty 2018-11-01 12:20:21 PDT
Created attachment 353634 [details]
artrace showing the process launch time for a system which has a Safari root with proposed changes
Comment 6 Suresh Koppisetty 2018-11-01 13:34:06 PDT
Created attachment 353638 [details]
Patch
Comment 7 EWS Watchlist 2018-11-01 14:36:08 PDT
Comment on attachment 353638 [details]
Patch

Attachment 353638 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9819080

New failing tests:
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign_after_load.html
Comment 8 EWS Watchlist 2018-11-01 14:36:10 PDT
Created attachment 353645 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Suresh Koppisetty 2018-11-01 16:09:45 PDT
Created attachment 353658 [details]
Patch
Comment 10 Suresh Koppisetty 2018-11-02 11:08:19 PDT
<rdar://problem/45736262> Dont use main queue to create an XPC connection as xpc_connection_set_bootstrap does a dispatch_mach_send_barrier_f on this queue which delays the sending of subsequent bootstrap message

Added artraces to the above radar for further reference.

cleanRoot.artrace		- Safari root with no changes	
DefaultDispatchSafari.artrace	- Safari root with the proposed changes. 	

Total Process Launch time changed from  330.49 ms to 271.58 ms
Process Launch time changed from 131.45 ms to 65.04 ms

Looks like a ~18% gain in the total process launch time and about ~50% in process launch time.
Comment 11 Geoffrey Garen 2018-11-02 14:02:14 PDT
Comment on attachment 353658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353658&action=review

I think the performance improvement is ready to go, but there are some details to get right before landing this patch.

> Source/WebKit/ChangeLog:10
> +        Donât use the main queue to create an XPC connection as xpc_connection_set_bootstrap does
> +        dispatch_mach_send_barrier_f on this queue which delays the sending of the subsequent bootstrap message.

Probably worth explaining that the bootstrap message is the message that launches the target process, so use of the main queue can delay process launch when the main queue is busy.

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:104
> +    // Dont use main queue to create an XPC connection as xpc_connection_set_bootstrap does a dispatch_mach_send_barrier_f on this queue which delays the sending of subsequent bootstrap message

I think comments work best when they describe what the code does (and why). Comments that describe what the code does not do, because doing so would have been a mistake, are harder to follow -- and, in theory, they are infinite. ("Don't dereference null here", "Don't use deprecated API here", "Don't use an N^2 algorithm here"...)

I'm not sure that we need any comment to explain why we accepted the default behavior of xpc_connection_create. Presumably, the default is usually an OK behavior.

I think I would remove this comment. The history of your decision making is still visible in bugzilla and svn.

Or, if you feel that a comment is warranted, I would explain affirmatively that nullptr has the effect of using the global concurrent queue for process launch, which can be an optimization when the main thread is busy.

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:105
> +    m_xpcConnection = adoptOSObject(xpc_connection_create(name, 0));

0 => nullptr

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:208
> +            if (!processLauncher->isLaunching())
> +                return;

Please use spaces instead of tabs, here and elsewhere. This diff looks larger than it should in svn history and patch review because of whitespace changes. (WebKit style uses spaces rather than tabs everywhere.)

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:236
>      if (UNLIKELY(m_launchOptions.shouldMakeProcessLaunchFailForTesting)) {
> -        RunLoop::main().dispatch([errorHandler = WTFMove(errorHandler)] {
> -            errorHandler(nullptr);
> -        });
> +        errorHandler(nullptr);
>          return;

This doesn't seem quite right. You've made the errorHandler sync instead of async -- but we want it to be async because it will be async in practice. Perhaps you were trying to remove use of the main thread here? But you haven't, because the sync invocation will happen on the main thread too. The truest to life behavior here would be to dispatch_async to dispatch_get_global_queue().

> Tools/ChangeLog:5
> +        Add "Total Process Launch Time".
> +        https://bugs.webkit.org/show_bug.cgi?id=191160
> +        <rdar://problem/45736262>

I think you meant to post this as a separate patch.
Comment 12 Suresh Koppisetty 2018-11-05 10:55:57 PST
Created attachment 353885 [details]
Patch
Comment 13 Suresh Koppisetty 2018-11-05 10:58:35 PST
(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 353658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353658&action=review
> 
> I think the performance improvement is ready to go, but there are some
> details to get right before landing this patch.
> 
> > Source/WebKit/ChangeLog:10
> > +        Donât use the main queue to create an XPC connection as xpc_connection_set_bootstrap does
> > +        dispatch_mach_send_barrier_f on this queue which delays the sending of the subsequent bootstrap message.
> 
> Probably worth explaining that the bootstrap message is the message that
> launches the target process, so use of the main queue can delay process
> launch when the main queue is busy.


Changing it to the following. 

        Don't use the main queue to create an XPC connection. xpc_connection_set_bootstrap does dispatch_mach_send_barrier_f on this queue which delays the sending of the subsequent bootstrap message (sent to launchd for launching a new target process) when the main queue is busy.


> 
> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:104
> > +    // Dont use main queue to create an XPC connection as xpc_connection_set_bootstrap does a dispatch_mach_send_barrier_f on this queue which delays the sending of subsequent bootstrap message
> 
> I think comments work best when they describe what the code does (and why).
> Comments that describe what the code does not do, because doing so would
> have been a mistake, are harder to follow -- and, in theory, they are
> infinite. ("Don't dereference null here", "Don't use deprecated API here",
> "Don't use an N^2 algorithm here"...)
> 
> I'm not sure that we need any comment to explain why we accepted the default
> behavior of xpc_connection_create. Presumably, the default is usually an OK
> behavior.
> 
> I think I would remove this comment. The history of your decision making is
> still visible in bugzilla and svn.

Right. Deleted the comment.

> 
> Or, if you feel that a comment is warranted, I would explain affirmatively
> that nullptr has the effect of using the global concurrent queue for process
> launch, which can be an optimization when the main thread is busy.
> 
> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:105
> > +    m_xpcConnection = adoptOSObject(xpc_connection_create(name, 0));
> 
> 0 => nullptr

Done. 

> 
> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:208
> > +            if (!processLauncher->isLaunching())
> > +                return;
> 
> Please use spaces instead of tabs, here and elsewhere. This diff looks
> larger than it should in svn history and patch review because of whitespace
> changes. (WebKit style uses spaces rather than tabs everywhere.)

There is an indentation change because I was dispatching the work using RunLoop::main().dispatch inside errorHandler function.

 RunLoop::main().dispatch([weakProcessLauncher, listeningPort] {
     ....
 });

> 
> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:236
> >      if (UNLIKELY(m_launchOptions.shouldMakeProcessLaunchFailForTesting)) {
> > -        RunLoop::main().dispatch([errorHandler = WTFMove(errorHandler)] {
> > -            errorHandler(nullptr);
> > -        });
> > +        errorHandler(nullptr);
> >          return;
> 
> This doesn't seem quite right. You've made the errorHandler sync instead of
> async -- but we want it to be async because it will be async in practice.
> Perhaps you were trying to remove use of the main thread here? But you
> haven't, because the sync invocation will happen on the main thread too. The
> truest to life behavior here would be to dispatch_async to
> dispatch_get_global_queue().

errorHandler runs in sync but it only does an assert and then dispatches the work using "RunLoop::main().dispatch" function. 

> 
> > Tools/ChangeLog:5
> > +        Add "Total Process Launch Time".
> > +        https://bugs.webkit.org/show_bug.cgi?id=191160
> > +        <rdar://problem/45736262>
> 
> I think you meant to post this as a separate patch.

Deleted this from this change.
Comment 14 Geoffrey Garen 2018-11-05 13:51:59 PST
> There is an indentation change because I was dispatching the work using
> RunLoop::main().dispatch inside errorHandler function.

I see.

This is a great thing to explain in your ChangeLog: Now that the XPC connection runs on the default concurrent queue, our errorHandler may be invoked by any thread.

I think you should try to make this distinction clearer in code. I would declare an errorHandler and an errorHandlerImpl. The job of errorHandler is to dispatch to the main thread to invoke errorHanderlImpl. If you do things that way, the diff will be smaller, and it will be clearer that the only job of errorHandler is to forward events to our preferred thread.
Comment 15 Babak Shafiei 2018-11-05 19:33:10 PST
The content of attachment 353628 [details] has been deleted for the following reason:

Error
Comment 16 Babak Shafiei 2018-11-05 19:33:29 PST
The content of attachment 353634 [details] has been deleted for the following reason:

Error
Comment 17 Babak Shafiei 2018-11-05 19:33:53 PST
The content of attachment 353633 [details] has been deleted for the following reason:

Error
Comment 18 Babak Shafiei 2018-11-05 19:34:08 PST
The content of attachment 353630 [details] has been deleted for the following reason:

Error
Comment 19 Suresh Koppisetty 2018-11-06 14:16:04 PST
Created attachment 354003 [details]
Patch
Comment 20 Geoffrey Garen 2018-11-06 15:53:21 PST
Comment on attachment 354003 [details]
Patch

r=me
Comment 21 WebKit Commit Bot 2018-11-06 16:19:21 PST
Comment on attachment 354003 [details]
Patch

Clearing flags on attachment: 354003

Committed r237901: <https://trac.webkit.org/changeset/237901>
Comment 22 WebKit Commit Bot 2018-11-06 16:19:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Saam Barati 2018-11-07 14:43:31 PST
Comment on attachment 354003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354003&action=review

> Source/WebKit/ChangeLog:11
> +        bootstrap message (sent to launchd for launching a new target process) when the main queue is busy.

Did this speed up process launch time?
Comment 24 Suresh Koppisetty 2018-11-07 14:53:01 PST
(In reply to Saam Barati from comment #23)
> Comment on attachment 354003 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354003&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        bootstrap message (sent to launchd for launching a new target process) when the main queue is busy.
> 
> Did this speed up process launch time?

Yes. Here are the numbers.

Total Process Launch time changed from  330.49 ms to 271.58 ms
Process Launch time changed from 131.45 ms to 65.04 ms

Looks like a ~18% gain in the total process launch time (includes initialization of web process) and about ~50% in process launch time.
Comment 25 Geoffrey Garen 2018-11-07 15:05:17 PST
> Total Process Launch time changed from  330.49 ms to 271.58 ms
> Process Launch time changed from 131.45 ms to 65.04 ms
> 
> Looks like a ~18% gain in the total process launch time (includes
> initialization of web process) and about ~50% in process launch time.

I believe this is as measured by the new tab benchmark. One thing that's unique about the new tab benchmark is that it opens a new tab, which can make the UI process's main thread busy with UI rendering work. (In theory, a perfectly idle app will not benefit from this change.)
Comment 26 Suresh Koppisetty 2018-11-07 15:23:34 PST
(In reply to Geoffrey Garen from comment #25)
> > Total Process Launch time changed from  330.49 ms to 271.58 ms
> > Process Launch time changed from 131.45 ms to 65.04 ms
> > 
> > Looks like a ~18% gain in the total process launch time (includes
> > initialization of web process) and about ~50% in process launch time.
> 
> I believe this is as measured by the new tab benchmark. One thing that's
> unique about the new tab benchmark is that it opens a new tab, which can
> make the UI process's main thread busy with UI rendering work. (In theory, a
> perfectly idle app will not benefit from this change.)

These numbers are mean values of corresponding signpost intervals in artrace while opening around 3 tabs in Safari. Please see <rdar://problem/45736262> for screenshots and traces.  

Here are the numbers from new tab benchmark:

Clean root :  
mean : 423 ms
Standard deviation : 17 ms

With Changes: 
mean : 396 ms
Standard deviation : 18 ms

For both artrace and new tab script, I have used two roots (one with the changes and one without the changes) to compare them.
Comment 27 Saam Barati 2018-11-08 00:32:20 PST
(In reply to Suresh Koppisetty from comment #24)
> (In reply to Saam Barati from comment #23)
> > Comment on attachment 354003 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354003&action=review
> > 
> > > Source/WebKit/ChangeLog:11
> > > +        bootstrap message (sent to launchd for launching a new target process) when the main queue is busy.
> > 
> > Did this speed up process launch time?
> 
> Yes. Here are the numbers.
> 
> Total Process Launch time changed from  330.49 ms to 271.58 ms
> Process Launch time changed from 131.45 ms to 65.04 ms
> 
> Looks like a ~18% gain in the total process launch time (includes
> initialization of web process) and about ~50% in process launch time.

Nice. That’s awesome. FWIW, this is the kind of data that’s good to put in the changelog entry.
Comment 28 Saam Barati 2018-11-08 00:35:27 PST
What are you defining as “Total Process Launch Time”? Is this from the time we create a new process up until that process has finished running the InitializeWebProcess (I forget what it’s actually called) command?
Comment 29 Suresh Koppisetty 2018-11-08 10:52:40 PST
(In reply to Saam Barati from comment #27)
> Nice. That’s awesome. FWIW, this is the kind of data that’s good to put in the changelog entry.

Sure. I will keep this in mind. 

(In reply to Saam Barati from comment #28)
> What are you defining as “Total Process Launch Time”? Is this from the time
> we create a new process up until that process has finished running the
> InitializeWebProcess (I forget what it’s actually called) command?

Yes. I am not a big fan of the name as its confusing with "Process Launch Time" but I wanted to define an interval which captures both process launch time and InitializeWebProcess phase.
Comment 30 Keith Miller 2019-01-20 15:27:05 PST
This appears to be a ~.3-.5% speedometer2 regression. Is there any way that we can work around that?
Comment 31 Chris Dumez 2019-01-20 16:28:27 PST
(In reply to Keith Miller from comment #30)
> This appears to be a ~.3-.5% speedometer2 regression. Is there any way that
> we can work around that?

This seems surprising. As far as I know, speedometer 2 should not be impacted by process launch at all.
Comment 32 Geoffrey Garen 2019-01-22 11:46:31 PST
> This appears to be a ~.3-.5% speedometer2 regression. Is there any way that
> we can work around that?

Is there an A/B task that confirms this regression?
Comment 33 Keith Miller 2019-01-22 12:50:22 PST
(In reply to Geoffrey Garen from comment #32)
> > This appears to be a ~.3-.5% speedometer2 regression. Is there any way that
> > we can work around that?
> 
> Is there an A/B task that confirms this regression?

Yeah, but I now realized that I bisected the wrong revision and it's only 80% confidence...