[RFC] Performance improvements

[Martin Schwenke]

Hi Swen,

On Thu, 21 Jun 2018 08:39:39 +0200, Swen Schillig via samba-technical
<samba-technical at lists.samba.org> wrote:

> On Wed, 2018-06-20 at 15:09 -0700, Jeremy Allison wrote:
> > On Wed, Jun 20, 2018 at 10:51:58PM +0200, Swen Schillig wrote:  
> > > 
> > > Therefore, I would really like you to have a look at the code again
> > > and
> > > "verify" if the changes are as evil as you claim.  
> > 
> > You do have some integer wrap and variable type
> > work to do first - so why not do that and then resubmit
> > with the "questionable" patch as the last entry
> > so it can be reviewed carefully individually ?
> >   
> 
> Thanks for all your support....and here we go.

As far as I can tell, commit 1 (ctdb: calculate queue input buffer size
correctly) reverts a non-trivial amount of the following commit:

  commit a61a4b1254e162f39ac22665868ec116a96815f3
  Author: Amitay Isaacs <amitay at gmail.com>
  Date:   Wed Aug 21 14:42:06 2013 +1000

    common/io: Limit the queue buffer size for fair scheduling via
    tevent

I'd like the commit message for commit 1 refer to the above commit
message, making sure that the issue addressed there is still addressed.

Similarly, commit 8 (ctdb: Increase queue's default buffer size to
8k) basically reverts the following commit (though with a different
number):

  commit 5dc18882fdbaed627262156dd453e6a8a2c0140f
  Author: Amitay Isaacs <amitay at gmail.com>
  Date:   Tue Jul 26 14:50:10 2016 +1000

    ctdb-daemon: Reduce QueueBufferSize from 16k to 1k

Again, I'd like to see reference to the above commit to ensure the
issue mentioned there is being considered.

So, while these 2 commits are likely to provide a performance
improvement, they might also introduce harmful side-effects that were
addressed by the 2 commits above from Amitay.

Commit 2 (ctdb: replace talloc / memcpy by talloc_memdup) is fine and
already Reviewed-by: Volker.  It isn't a performance improvement but is
reasonable cleanup, so I'll just give it a:

  Reviewed-by: Martin Schwenke <martin at meltin.net>

and push now to get it out of this list of commits.

Why isn't the destructor in commit 3 (ctdb: remove queue destructor as
it isn't needed anymore) needed anymore?  Is it due to one of the
previous commits?  Or hasn't it been needed for a while?  If the
latter, is there a particular commit that stops it from being needed?
My guess is this one:

  commit e097b7f8ff1a9992de1d11474dac4969e30cd679
  Author: David Disseldorp <ddiss at suse.de>
  Date:   Sun Jul 31 03:14:54 2011 +0200

    io: Make queue_io_read() safe for reentry

But I'm not sure.  If you've done the analysis then please share it so
that the reviewer doesn't have to do it again.  :-)

I doubt that commit 6 (ctdb: Consolidate all ALIGNMENT
instructions) provides any performance benefits.  Is it needed as part
of this patch set?  If not, I'd still like to take time to consider it
separately when time permits.

OK, that leaves 3 (other sets of) changes that improve performance.

Commit 4 (ctdb: Introduce buffer.offset to avoid memmove) adds an
offset into the buffer, similar to the begin offset in sock_io.c.  That
seems like a good idea.

I know I'm strange, but I'd rather be bored by seeing relevant fields
explicitly zeroed instead of:

	memset(&queue->buffer, 0, sizeof(queue->buffer)); 

which I have to decode to understand why it is being done.

I'm also not sure why you dropped the "failed:" goto label and inlined
that instead.  It just makes the patch bigger.  :-(

Commits 5, 7 & 9 (ctdb: Adding memory pool for queue callback, ctdb:
Introduce tunable parameter for queue mempool size, ctdb: Enabling
ctdb_queue_send to use memory from data_pool) add a memory pool to
reduce the number of malloc()s.  Seems like a reasonable plan.

In commit 5 please don't use CTDB_NO_MEMORY_NULL().  We would like to
get rid of that and similar macros.  Used lazily, they cause memory
leaks.

I'm hoping we can do without commit 7.  We need less tunables instead
of more.  :-)  The plan is to get rid of tunables completely, replacing
them with configuration file settings.  Hopefully there is a number that
generally works. I suspect that without some very good advice in the
documentation, most users won't know what to do with it anyway and will
never use it.

Like others, I don't think commit 10 is a good idea.  It replaces
processing the next packet via a scheduled event with inline processing
of the next packet.  That gives no chance for the event loop to do
anything.  At the moment it looks like at least tevent signal handlers
can be processed.  As I understand it, the desirable model is to go back
to the event loop as frequently as possible and do work in small
increments and this change works against that model.

So, my question is: how much improvement do you get with just commits
4, 5 and 9?

peace & happiness,
martin