[RFC] Performance improvements

[Swen Schillig]

Hi Martin

... seems we're getting closer.

On Wed, 2018-07-25 at 18:42 +1000, Martin Schwenke wrote:
> Hi Swen,
> 
> On Thu, 19 Jul 2018 10:06:40 +0200, Swen Schillig <swen at vnet.ibm.com>
> wrote:
> 
> > We only read up-to (configured) buffer_size from the socket and not
> > what could be read. This is the original behaviour but the code got
> > modified to keep as much as possible of the improvements 
> > (e.g. direct mem size calculation and not jumping back and forth
> > until
> > we have the right size)
> 
> It took me a couple of hours to understand this patch.
I'm sorry, I thought it is totally obvious what it does.

> 
> It does several things:
> 
> 1. Drops the "extend" buffer element and does the required
> calculation
>    inline to avoid multiple allocations for a packet
> 
> 2. Introduces a variable "qb" to improve readability
> 
> 3. Drops the "navail" variable and uses a MIN() calculation instead
> 
> 4. Changes the conditions for the default buffer allocation
> 
> 5. Avoids one use of "goto failed".
> 
> When I finally understood this, I wondered if I could accomplish the
> goal (i.e. #1), more easily.
Hmmm, you're pretty much doing what my patch is doing.
But I still believe my patch is much easier to read and cleaner,
e.g. even if we meet all requirements to read "num_ready" of the socket
we still have to perform 4 checks with your version and only one with
mine....if the space is not sufficient we need to do the same.
I like the separation of let's name it good-path / bad-path, but hey.

> 
> Does the attached patch do this?  It seems to work but I haven't
> spent
> a lot of time trying to make sure it is correct.
Well it isn't fully,
your patch leaks the memory of the "old data" if the realloc fails.

> 
> The point I'm trying to make is that this patch was *really* hard to
> review because it made a bunch of changes that aren't necessary for
> the
> optimisation you're interested in.  Some of those other changes might
> be valid clean-ups (e.g. qb) but putting them in the same patch as
> the
> optimisation just makes it hard to understand what you're really
> trying
> to do.  Simple decisions like using a new variable name m_length
> instead
> of using the same pkt_size variable as in the previous function

Ok, I will change that name to "pkt_size".

>  require multiple looks because they don't follow the established
> pattern.
Hmm, fair point. 

But...

If one is re-doing the code...and I believe I am here.....then I cannot
keep the obvious changes (you call them clean-ups) to a separate patch.
I'm prepare to do another round of modifications to the patches but
after that I will pull-back
Along with my suggested modifications I added some more comments as
well, which in turn make my patch larger than yours.
In addition I knew of the updates to come in the succeeding patches
which made me prepare here and there already.
..and you said it in your introduction where you listed my
modifications...."introduce qb to improve readability"

Do you really think I should put those updates to an addtl. patch ?
...that would mean cleanup-patches and people were cursing me already
for sending those !

Besides, I'm not 100% sure if I understand your motivation for making
your own "patch-1" version doing the same thing but leaving out a fix
and some code readability improvements.

I've spend already a lot of time on those (remaining) updates
and we both know of people who think already too much time.
I might be prepare to do another round of small updates to my
modifications, so what is you final call ?

Cheers Swen