[RFC] Performance improvements
jra at samba.org
Wed Jun 20 16:38:23 UTC 2018
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.
If you don't do this, and process multiple events in one
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.
Now this may be safe in ctdb in this particular case, but
it's an aberrant use of tevent that has great potential
to cause future bugs, so on those grounds I would NAK
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...
More information about the samba-technical