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

[Tanu Kaskinen]

On Wed, 2013-04-10 at 08:50 +0200, David Henningsson wrote:
> On 04/08/2013 06:27 PM, Tanu Kaskinen wrote:
> > 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.
> >
> 
> For the record, I've pushed the write patch to both branches now.
> 
> I'm having second thoughts about the read patch. Perhaps it's better to 
> introduce an extra layer of buffering instead? E g, pa_iochannel_read 
> would then never call pa_read with anything less than, say, 256 bytes. 
> (For big reads, there would still be no memory copy.)

Reading always at least 256 bytes at a time sounds like a good idea, but
I don't immediately see how it would work in practice. Just adding some
buffering in the iochannel doesn't help much. It may save a read()
syscall (if libc doesn't already do some extra buffering), but if
do_read() isn't modified in any way, the extra ppoll() call will still
be there.

-- 
Tanu