Bug 191160 – Don’t use the main queue to create an XPC connection

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Suresh Koppisetty]

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

[Suresh Koppisetty]

Created attachment 353626 [details]
Patch

[Suresh Koppisetty]

Created attachment 353628 [details]
artrace showing the distribution of total process launch time for a system which has a Safari root with no changes

[Suresh Koppisetty]

Created attachment 353630 [details]
artrce screenshot showing total process launch time with a Safari root with proposed changes

[Suresh Koppisetty]

Created attachment 353633 [details]
artrace showing the process launch time for a system which has a Safari root with no changes

[Suresh Koppisetty]

Created attachment 353634 [details]
artrace showing the process launch time for a system which has a Safari root with proposed changes

[Suresh Koppisetty]

Created attachment 353638 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Suresh Koppisetty]

Created attachment 353658 [details]
Patch

[Suresh Koppisetty]

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

[Geoffrey Garen]

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.

[Suresh Koppisetty]

Created attachment 353885 [details]
Patch

[Suresh Koppisetty]

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

[Geoffrey Garen]

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

[Babak Shafiei]

The content of attachment 353628 [details] has been deleted for the following reason:

Error

[Babak Shafiei]

The content of attachment 353634 [details] has been deleted for the following reason:

Error

[Babak Shafiei]

The content of attachment 353633 [details] has been deleted for the following reason:

Error

[Babak Shafiei]

The content of attachment 353630 [details] has been deleted for the following reason:

Error

[Suresh Koppisetty]

Created attachment 354003 [details]
Patch

[Geoffrey Garen]

Comment on attachment 354003 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 354003 [details]
Patch

Clearing flags on attachment: 354003

Committed r237901: <https://trac.webkit.org/changeset/237901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

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?

[Suresh Koppisetty]

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

[Geoffrey Garen]

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

[Suresh Koppisetty]

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

[Saam Barati]

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

[Saam Barati]

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?

[Suresh Koppisetty]

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

[Keith Miller]

This appears to be a ~.3-.5% speedometer2 regression. Is there any way that we can work around that?

[Chris Dumez]

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

[Geoffrey Garen]

> 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?

[Keith Miller]

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