[pulseaudio-discuss] [PATCH 2/2] pstream: Optimise read of smaller packages

[Tanu Kaskinen]

On Sun, 2013-04-07 at 11:59 +0200, David Henningsson wrote:
> On 04/05/2013 09:44 PM, David Henningsson wrote:
> > During a stream, most packets sent are either memblocks (with SHM info),
> > or requests for more data. These are only slightly bigger than the
> > header.
> >
> > This patch makes it possible to read these packages in one call to do_read,
> > thus saving us an extra poll syscall.
> >
> > Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> > ---
> >   src/pulsecore/pstream.c |    8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> > index d92178f..c6f1302 100644
> > --- a/src/pulsecore/pstream.c
> > +++ b/src/pulsecore/pstream.c
> > @@ -27,6 +27,7 @@
> >   #include <stdio.h>
> >   #include <stdlib.h>
> >   #include <unistd.h>
> > +#include <errno.h>
> >
> >   #ifdef HAVE_NETINET_IN_H
> >   #include <netinet/in.h>
> > @@ -652,6 +653,7 @@ static int do_read(pa_pstream *p) {
> >       pa_assert(p);
> >       pa_assert(PA_REFCNT_VALUE(p) > 0);
> >
> > +again:
> >       if (p->read.index < PA_PSTREAM_DESCRIPTOR_SIZE) {
> >           d = (uint8_t*) p->read.descriptor + p->read.index;
> >           l = PA_PSTREAM_DESCRIPTOR_SIZE - p->read.index;
> > @@ -774,6 +776,10 @@ static int do_read(pa_pstream *p) {
> >               }
> >           }
> >
> > +        /* Optimisation: See if we can read the payload too */
> > +        if ((p->read.data || p->read.memblock) && (r == PA_PSTREAM_DESCRIPTOR_SIZE))
> > +            goto again;
> > +
> >       } else if (p->read.index > PA_PSTREAM_DESCRIPTOR_SIZE) {
> >           /* Frame payload available */
> >
> > @@ -894,6 +900,8 @@ fail:
> 
> It's probably better to do:
> 
>   if (r == -1 && (errno == EAGAIN || errno == EWOULDBLOCK))
>      r = 0;
>   else
>      r = -1;
> 
> >       if (release_memblock)
> >           pa_memblock_release(release_memblock);
> >
> 
> return r;
> 
> That way we don't have to make the assumption that pa_memblock_release 
> can't destroy errno.

Yes.

I also have some reservations about using goto to backwards in a complex
function. I'm not sure, however, that using a proper loop structure
would make the code any more readable. Splitting the function into
smaller functions is something to consider, but I'm not demanding that
if you don't think it's a good idea.

-- 
Tanu