[webkit-reviews] review denied: [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 : [Attachment 353658] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Suresh Koppisetty
<skoppisetty at apple.com>'s request for review:
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
https://bugs.webkit.org/show_bug.cgi?id=191160

Attachment 353658: Patch

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




--- Comment #11 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 353658
  --> https://bugs.webkit.org/attachment.cgi?id=353658
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.