[Webkit-unassigned] [Bug 191160] 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 subsequent bootstrap message

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=191160

--- Comment #13 from Suresh Koppisetty <skoppisetty at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181105/6c079257/attachment-0001.html>