[pulseaudio-discuss] Vacuuming per-client mempools (was: [PATCH v2 02/12] pulsecore: srbchannel: Introduce per-client SHM files)

[Tanu Kaskinen]

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