[RFC] Performance improvements

[Swen Schillig]

On Wed, 2018-06-20 at 09:38 -0700, Jeremy Allison wrote:
> On Wed, Jun 20, 2018 at 11:40:58AM +0200, Swen Schillig via samba-
> technical wrote:
> > Hi Volker
> > 
> > Thanks for having a look.
> > 
> > On Wed, 2018-06-20 at 11:22 +0200, Volker Lendecke wrote:
> > > On Mon, Jun 18, 2018 at 10:04:03AM +0200, Swen Schillig via
> > > samba-
> > > technical wrote:
> > > > Attached is the patchset in question.
> > > > Even though the set consists of 10 patches I wouldn't call them
> > > > all
> > > > micro-patches..but they're as small as possible I guess.
> > > 
> > > 2/10 is an obvious RB+ by me.
> > > 
> > > 10/10 is really, really fishy. We should never handle more than
> > > one
> > > event from a single tevent_loop_once. Not a formal NACK, but this
> > > needs thorough review by people more familiar with the core of
> > > ctdb's
> > > request handling.
> > 
> > After our conversation this morning about this, I was checking
> > again.
> > 
> > Actually I'm not handling more than one event, if you look again
> > you
> > will see that I'm just NOT creating an additional event by calling
> > tevent_schedule_immediate for the data already in the buffer 
> > ...instead I process the data right away.
> > 
> > So I think this is the tiny but important difference.
> 
> That usually is the wrong thing to do. As Volker pointed
> out, tevent is designed around single-event-processing
> and then returning into the loop.

I do process a single event only as you can clearly see by looking at
the code.
> 
> If you don't do this, and process multiple events in one
> callback, 
... and each packet is handled by an individually triggered callback.

> if part of the event processing schedules an
> immediate event (or socket activation etc.) then you can
> accidently trigger out-of-order event handling, which can
> lead to horrible to find bugs.

...and there is NO out of order handling.

> 
> Now this may be safe in ctdb in this particular case, but
> it's an aberrant use of tevent t
It is totally transparent to tevent....one event ... one invocation,
no messing with multiple events in one go.

> hat has great potential
> to cause future bugs, so on those grounds I would NAK
> this.
I think you're a bit too quick with your judgement, is there 
a test-case which shows/proves the issue you saw in the past ?

> 
> Do you have any numbers showing this patch is essential
> for the perforance gain (i.e. without this patch do you
> still see performance improvements).
> 
> I'd rather avoid such "strange" tevent use if we don't
> need it. It's another matter if doing this is the core
> of the performance gain of course...
well, since it's the "last" patch out of the 10 I posted, 
I guess it is not so difficult to have a start without it.
...I think I could separate out just this part from the last patch and
keep the other updates again in an individual patch.

Even though I cannot quantify the improvement of that patch I would
still believe that it has its share on the reduction of the variation.

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.

Thanks again for your support.

Cheers Swen