On Fri, 2016-02-12 at 08:08 +0200, Ahmed S. Darwish wrote:
> On Fri, Feb 12, 2016 at 02:09:31AM +0200, Ahmed S. Darwish wrote:
> > The PA daemon currently uses a single SHM file for all the clients
> > sending and receiving commands over the low-latency srbchannel
> > mechanism.
> >
> > To safely run PA daemon in system mode later using memfds, and to
> > provide the necessary ground work for sandboxing, create the
> > srbchannel SHM files on a per-client basis.
> >
> ...
> > @@ -285,8 +278,16 @@ void pa_core_maybe_vacuum(pa_core *c) {
> > Â
> > Â Â Â Â Â pa_mempool_vacuum(c->mempool);
> > Â
> > -Â Â Â Â if (c->rw_mempool)
> > -Â Â Â Â Â Â Â Â pa_mempool_vacuum(c->rw_mempool);
> > +Â Â Â Â /*
> > +Â Â Â Â Â * FIXME: Vacuum the per-client rw (srbchannel) mempools
> > +Â Â Â Â Â *
> > +Â Â Â Â Â * We need to vacuum the per-client mempool owned by each connection
> > +Â Â Â Â Â * at pa_native_protocol::connections. Nonetheless, libpulsecore can
> > +Â Â Â Â Â * not access any symbol from protocol-native.
> > +Â Â Â Â Â *
> > +Â Â Â Â Â * Vacuuming a per-client mempool on its streams deletion and
> > +Â Â Â Â Â * uncorking also proved to be problematic.
> > +Â Â Â Â Â */
> > Â }
> >
>
> During patch review of v1 submission, a consensus was reached to
> vacuum the per-client mempool according to streams activity. [*]
>
> So the logic goes:
>
> - Upon corking a stream, if there are no more 'active' streams, let's
> Â vacuum per-client mempool
>
> - Upon deleting a stream, if all the remaining streams are not active
> Â (corked) or no more streams left, vacuum
>
> - Upon finishing an upload stream, if no other streams remain or all
> Â are inactive, vacuum
>
> When implementing this seemingly-innocent logic, I've found that we
> are vacuuming way too much.Â
>
> Here's a case-by-case analysis:
>
> #####################################################################
>
> 1) GNOME music, official gnome3 music application
> Â Â Â https://wiki.gnome.org/Apps/Music
>
> Â Â Â After starting application, _clicking_ a song to play we get:
>
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = pulsesink probe
> Â Â Â Â Â Â Â DELETING stream with name = pulsesink probe
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = Playback Stream
> Â Â Â Â Â Â Â UN_CORKING stream with name = Playback Stream
> Â Â Â
> Â Â Â Whenever the app _transitions_ from song to song, we get:
>
> Â Â Â Â Â Â Â CORKING stream with name = Playback Stream
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
>        CORKING stream with name = Playback Stream      Â
> Â Â Â Â Â Â Â DELETING stream with name = Playback Stream
> Â Â
>
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = pulsesink probe
> Â Â Â Â Â Â Â DELETING stream with name = pulsesink probe
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = Playback Stream
> Â Â Â Â Â Â Â UN-CORKING stream with name = Playback Stream
>
> Â Â Â When _seeking_ a playing song, we get:
>
> Â Â Â Â Â Â Â CORKING stream with name = Playback Stream
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
> Â Â Â Â Â Â Â UN-CORKING stream with name = Playback Stream
>
> Â Â Â FINAL RESULT:
> Â Â Â Â Â Â Â 2 vacuums per song transition
> Â Â Â Â Â Â Â 1 vacuum per song seek
> Â Â Â Â Â Â Â
> #####################################################################
>
> 2) Audacious, XMMS successor, http://audacious-media-player.org/
> Â Â Â Audacious uses ALSA output by default.
>
> Â Â Â After starting application, clicking a song to play we get
>
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = ALSA Playback
> Â Â Â Â Â Â Â DELETING stream with name = ALSA Playback
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = ALSA Playback
> Â Â Â Â Â Â Â UN-CORKING stream with name = ALSA Playback
>
> Â Â Â Whenever the app transitions from song to song, we get:
>
> Â Â Â Â Â Â Â CORKING stream with name = ALSA Playback
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
> Â Â Â Â Â Â Â DELETING stream with name = ALSA Playback
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = ALSA Playback
> Â Â Â Â Â Â Â UN-CORKING stream with name = ALSA Playback
>
> Â Â Â When seeking a playing song, we get:
>
> Â Â Â Â Â Â Â CORKING stream with name = ALSA Playback
> Â Â Â Â Â Â Â <----------------- VACUUM ------------------->
> Â Â Â Â Â Â Â DELETING stream with name = ALSA Playback
> Â Â Â Â Â Â Â CREATING/CONNECTING stream with name = ALSA Playback
> Â Â Â Â Â Â Â UN-CORKING stream with name = ALSA Playback
>
> Â Â Â FINAL RESULT:
> Â Â Â Â Â Â Â 1 vacuum per song transition
> Â Â Â Â Â Â Â 1 vacuum per song seek
>
> #####################################################################
>
> So unfortunately it seems this approach will produce a vacuuming
> overkill and possibly hurt our performance.
Have you checked how much time the vacuuming takes? I'm not sure the
frequent vacuuming is a real problem.
> The original logic depended on the core.c to detect that all sources
> and sinks are idle, and if so, vacuum all the server mempools. Trying
> to maintain this logic faces the issue of libpulsecore not being
> able to access any symbol from protocol-native.
If there's only one application playing (which is the common case), is
there any difference in behaviour with the old code compared to the
new? If there's just one stream, corking/deleting will still cause
vacuuming also with the old code, right?
> So what is the best way to handle this issue?
If it needs handling, I guess we could add a timer that triggers
vacuuming only if the client is still idle after a second or so.
--Â
Tanu
