1792474 - Avoid the IO thread hop when sending IPC messages

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently whenever we send a message over IPC, we need to do a thread-hop to the IO thread to actually write the relevant data out to the underlying pipe, due to the IPC::Channel mechanism not being threadsafe. This is possible to avoid in most cases by making the sending part of IPC::Channel threadsafe, and delaying the actual watching of the channel for space to become available to the IO thread.

This could benefit us in a few ways:

1. This should reduce the total cross-process latency for sending/receiving a message. This will still likely be dominated by the receiving thread's event loop being unavailable, but may help in some cases, especially when talking to idle background threads.

2. This will reduce how frequently the IO thread is woken, by only waking it up when receiving messages or for large message backlogs. The IO thread is currently one of the most frequently woken threads in the browser, which has a negative impact on our power usage.

When we do this we'll want to be extra careful about lifecycle expectations of both code using IPC::Channel and IPC::Channel itself, to make sure we don't introduce edge-case bugs, especially around channel shutdown.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1792474 - Part 1: Add an IsOnCurrentThread() helper to EventTargetCapability, r=#xpcom-reviewers

The type will be used to explicitly track which members of
IPC::Channel::ChannelImpl can only be accessed on the I/O thread vs. using the
mutex.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1792474 - Part 2: Remove a couple of unused IPC::Channel members, r=#ipc-reviewers

Depends on D158159

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1792474 - Part 3: Stop using IPC::Channel to create the pipe for ForkServer, r=#ipc-reviewers

Previously the channel used by the ForkServer would be created using
IPC::Channel, and then stolen after the launch was successful. Unfortunately,
this required invoking IPC::Channel methods (such as Close()) from the wrong
thread, and so would be racy and hit assertions with the new checks being
added. This patch instead skips creating the IPC::Channel for the fork server,
and allows it to create and configure its own pipe as needed.

This may be used in the future to change out the IPC strategy for the fork
server to something more appropriate, which supports features like async
replies as forked processes die.

Depends on D158160

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1792474 - Part 4: Avoid the IO thread hop when sending IPC messages, r=#ipc-reviewers

This involves some changes to IPC::Channel::ChannelImpl on all platforms in
order to ensure that they are threadsafe.

1. The ChannelImpl is now internally refcounted, allowing it to potentially be
   kept alive past Close() in some cases (such as when there's a pending
   runnable to the IO thread, or when there's outstanding OVERLAPPED io).
2. The windows ChannelImpl no longer blocks until all IO is completed in
   Close(), and instead relies on an extra reference taken when is_pending
   to keep the buffers alive.
3. Members of the channel are all annotated with MOZ_GUARDED_BY for either
   the io_thread_ or mutex_ depending on if they are required in order to
   send a message. This gives us some static checks that we won't deadlock.
4. The closed_ field is removed, as thanks to the mutex, pipe_ can now be
   checked directly from any thread instead. This reduces the risk of
   forgetting to update closed_.
5. NodeChannel now calls Send() without dispatching, which also required
   updating some other members to also be accessible from any thread, including
   changes to allow asynchronously reporting a channel error when Send()
   fails.
6. The Windows handling for Connect() was made more thread-safe to queue
   calls to Send() performed before Connect() returns. The posix
   Connect() handler already did this.

Depends on D158161

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d99eb822910
Part 1: Add an IsOnCurrentThread() helper to EventTargetCapability, r=xpcom-reviewers,kmag
https://hg.mozilla.org/integration/autoland/rev/6097fc43d925
Part 2: Remove a couple of unused IPC::Channel members, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/6909a77b7bd0
Part 3: Stop using IPC::Channel to create the pipe for ForkServer, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/1807a36ff99f
Part 4: Avoid the IO thread hop when sending IPC messages, r=ipc-reviewers,jld

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4d99eb822910
https://hg.mozilla.org/mozilla-central/rev/6097fc43d925
https://hg.mozilla.org/mozilla-central/rev/6909a77b7bd0
https://hg.mozilla.org/mozilla-central/rev/1807a36ff99f

Comment 7

[Andrej (:aglavic)]

== Change summary for alert #35647 (as of Mon, 10 Oct 2022 18:50:28 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
9%	tscrollx	linux1804-64-shippable-qr	e10s fission stylo webrender	1.37 -> 1.25
5%	tscrollx	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	1.11 -> 1.06

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35647