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

[David Henningsson]

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


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic