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

[Ahmed S. Darwish]

Hi!

On Fri, Feb 12, 2016 at 01:03:39PM +0200, Tanu Kaskinen wrote:
> 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:
> > 
> > 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
> >
..
> >    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.
> >
...
> >    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.
>

Nope, I did not, and I guess I should..

Beside direct measurement though, I'm afraid a bit of any ripple
effects. Following the code, "Vacuuming" is basically:

    madvise(ptr, size, MADV_REMOVE);
    madvise(ptr, size, MADV_DONTNEED);

And checking the manpage on MADV_DONTNEED, it states:

    MADV_DONTNEED: Do  not  expect  access  in  the near future.
    (For the time being, the application *is finished* with the
    given range, so the kernel can free resources associated with
    it.)

"Do not expect access in the near future" is definitely not what
happens when we are pressing "next song" on a GUI application, or
when we are doing a simple seek..

That's why I much prefer your second suggestion: "a timer that
triggers vacuuming only if the client is still idle after a
second or so". This clearly shows that we do not expect access
in the near future.

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

AFAIK no..

New code: I continuously play 4 songs in Gnome music ==> 7 vacuums
Old code: I play the same 4 songs                    ==> 0 vacuums

New code: Doing several seeks (3) in Totem video player until I
          find that movie segment                    ==> 3 vacuums
Old code: Doing the smae amount of seeks             ==> 0 vacuums

So I guess there's a big change.

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

+1

This matches the old logic and respects our contract with the
kenrel VM much more.

Regards,

-- 
Darwish
http://darwish.chasingpointers.com