[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 bugzilla-daemon at webkit.org
Fri Nov 2 14:02:14 PDT 2018


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

Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #353658|review?                     |review-
              Flags|                            |

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

-- 
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/20181102/8ea323b2/attachment-0001.html>


More information about the webkit-unassigned mailing list