[RFC] Performance improvements

[Andrew Bartlett]

On Wed, 2018-07-25 at 12:40 +0200, Swen Schillig via samba-technical
wrote:
> 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.

G'Day Swen,

One of the hard to learn lessons of Samba is that what matters is not
what each of us thinks about our own work, but that we are able to
communicate that to others.  Each brings their own perspectives,
backgrounds and history, but good code - really good patches -
communicates well to everyone slowly and patiently what is going on,
and why. 

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

It is really, really hard, but also really, really worthwhile to to
that.  I've leant that the hard way, as has everyone else with many
patches in Samba (because the alternative is patch purgatory forever as
your patches sit un-reviewed...).

Have a look at Gary's patch series to rename the ldb key_value layer. 
Each patch does the search/replace stage of the rename, then reformat
the lines.  Painful!  But also much easier to clearly see that each
patch only does one thing.

Metze has a rule:  never say 'and' in a commit message.  If you say
'and' it should be two commits.  Perhaps best taken as a guideline, and
sometimes not appropriate, but a worthy point to consider. 

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

Yes.  Please take a step back and understand why we asked you to do
that.  It wasn't to ask you to make 'real changes with cleanup'!  That
is even worse than 'pointless' clean-up patches, by a massive margin!

The advice was to was to focus on core things that make a difference,
because if you have a budget of X points of code review, don't waste
them all on 100 cleanups and then wonder why folks are exhausted of
reviewing your code before they get to the real change.   

When you finally have the confidence (and frankly, it is clear you
don't yet) of those with an interest in that area, and demonstrated
experience in doing clean-up well and in a way that everybody agrees is
an improvement, then you will find much less push-back. 

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

There are many, many good reasons to prepare patches that just fix the
issue.  Such patches are inherently smaller, easier to prove and easier
to follow.  The cleanup should be before or after, if at all possible. 

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

Swen,

Please step back and try and work on smaller, minimal changes that
easily pass review.  I've looked at your patch and Martins, and I can
follow his trivially, whereas yours is really hard to follow.  

Finally remember that Samba is Git patches as performance art.  Each is
to be carefully prepared, polished and presented with love, as a hand-
crafted gift to the future.  The beauty is in the stream of individual
gems.  It often feels overkill, but here today we see why we have the
emphasis, because it means that anybody - even someone outside CTDB
expertise, can say 'sure, that makes sense', and review it. 

Thanks,

Andrew Bartlett
-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba