[RFC] Performance improvements

[Martin Schwenke]

On Wed, 25 Jul 2018 12:40:08 +0200, Swen Schillig <swen at vnet.ibm.com>
wrote:

> On Wed, 2018-07-25 at 18:42 +1000, Martin Schwenke wrote:

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

You wrote it and you have been looking at this code in detail.  Nobody
else has.  Please helper the reviewers.

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

I'm not sure if you're talking about the patch itself or the resulting
code... or the existing code!

Your patch:

 ctdb/common/ctdb_io.c | 85 +++++++++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 36 deletions(-)

My patch:

 ctdb/common/ctdb_io.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

We are reviewing each patch individually.  A simpler patch that makes
less changes is easier to review.  You *might* need supporting patches
that provide scaffolding to make the final change simpler and easier to
understand.  If required, they should be simple too.  Later, when others
are looking at the commit history they will also want to be scanning
through simple patches.

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

My patch doesn't leak memory.  It behaves the same way as the existing
code.  Perhaps you found a bug in the existing code?  If you did then
please fix that bug and don't bury the fix in a performance
improvement!

However, there is another bug in my patch.  The condition for the
realloc is wrong.

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

Thanks.

> 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

Why are you re-doing the code?  If you want to improve performance then
please just do that.  I have invested my time to show that the desired
improvement can be made without re-doing the code.  

However, if you want to change the style of the code then please do
that in separate patches.  If people don't agree then the stylistic
changes can at least be NACKed separately.  I suspect that those
familiar with the current code don't want its style changed because it
probably follows a pattern that they are familiar with.  If they come
back to this code and find it changed then they need to relearn it, so
the style changes actually hinder others.

For example, you seem to dislike the "goto failed" in a couple of the
existing functions and your patches slowly but silently get rid of this.
There's no benefit here except personal preference and I don't see the
change as an improvement.  With the goto, the callback is obviously
called with the same arguments on each failure.  However, if you inline
that call then the reader needs to examine the arguments each time to
understand what is happening.  The number of lines might stay roughly
the same but the result is more complex.  So, NACK.

> Along with my suggested modifications I added some more comments as
> well, which in turn make my patch larger than yours.

2 of 28 additional lines.

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

I don't think any of the stylistic changes are needed in succeeding
patches.  I was trying to characterise the change rather than agree
with it.

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

Yes.  As I said above, at least we can discuss them separately and they
can be rejected if nobody agrees that they make the code better.

> 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 think I've covered that.

> 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 don't think I've seen people post such an opinion on the list. Please
ask those people to do that if is relevant to the technical discussion
going on here.

> I might be prepare to do another round of small updates to my
> modifications, so what is you final call ?

I don't want to spend 2 hours reviewing a simple performance
improvement because it contains spurious, obfuscating changes.

As I've said before, I want to review simple and obvious patches... and
I also want to see data about the performance improvement that indicates
that this conversation is worth having...

peace & happiness,
martin