1757802 - Consider no longer directly associating Shmem instances with actors

Status: REOPENED

(Core :: IPC, enhancement, P2)

Description

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

With this new approach, Shmem instances will now have their handles transferred inline within messages as attachments, rather than being associated with their actors and sent in separate messages.

This has a few advantages:

• The implementation is much simpler
• Releasing all references to a Shmem will automatically destroy it by RAII, rather than leaking the shared memory region until the toplevel actor is destroyed, removing the need for types like RaiiShmem.
• This allows re-transmitting Shmem instances to another process, as we don't close the shared memory region handle upon receiving it.

But also has a disadvantage that because we keep alive the shared memory region's handle until the shmem is destroyed, so that it can be re-transmitted, we may end up using more FDs or HANDLEs while running.

This bug intentionally doesn't change or simplify callsites, removing APIs like RaiiShmem, in order to make it easier to revert if this causes issues on platforms like Linux due to FD exhaustion. If we don't run into increased resource exhaustion problems, we can make these changes in a follow-up.

Comment 1

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

Attachment: Bug 1757802 - Don't keep alive Shmem shared memory regions on IProtocol, r=#ipc-reviewers

With this new approach, Shmem instances will now have their handles
transferred inline within messages as attachments, rather than being
associated with their actors and sent in separate messages.

This has a few advantages:

• The implementation is much simpler
• Releasing all references to a Shmem will automatically destroy it by
  RAII, rather than leaking the shared memory region until the toplevel
  actor is destroyed, removing the need for types like RaiiShmem.
• This allows re-transmitting Shmem instances to another process, as we
  don't close the shared memory region handle upon receiving it.

But also has a disadvantage that because we keep alive the shared memory
region's handle until the shmem is destroyed, so that it can be
re-transmitted, we may end up using more FDs or HANDLEs while running.

This patch intentionally doesn't change or simplify callsites, removing
APIs like RaiiShmem, in order to make it easier to revert if this causes
issues on platforms like Linux due to FD exhaustion. If we don't run
into increased resource exhaustion problems, we can make these changes
in a follow-up.

Depends on D140009

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d23532d6a49
Don't keep alive Shmem shared memory regions on IProtocol, r=ipc-reviewers,jld

Comment 3

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0d23532d6a49

Comment 4

[Atila Butkovits]

Backed out as requested.

Backout link: https://hg.mozilla.org/integration/autoland/rev/88a76c0a8d1cd349786eb1610cfa2f10907d7a1c

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/88a76c0a8d1cd349786eb1610cfa2f10907d7a1c

Comment 6

[Release mgmt bot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 7

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

There was a regression from the change and we still don't have any idea as to what caused it, so there's not a ton of immediate action we can take.

Comment 8

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

Err actually nevermind, wrong patch. This is blocked on bug 1767514 (and potentially other changes) for now.

Comment 9

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

We still need the ffmpeg update to land something like this, and we may want to also be a bit more clever around the media code etc.

Comment 10

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

(also around graphics potentially)

Comment 11

[Paul Adenot (:padenot)]

Nika, I've updated our vendored ffmepg copy to a recent version, containing the bug fix that was needed for this to not crash, maybe we can reland this?

Comment 12

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

We're slightly concerned about potential performance issues which might occur due to GMP code expecting the existing behaviour's performance characteristics, so we've been holding off on landing something like this directly. Instead, we've been adding some new Shmem types like BigBuffer or the flexible API for unsafe shred memory from bug 1795311 which are hopefully better suited for the existing uses of Shmem. We might want to then refactor the code out of the core IPC layer, and make it something which GMP deals with directly instead.