[Fwd: Re: [PATCH] Minor cleanup of libnet_LookupName_recv] || 2nd reviewer, please

[Swen Schillig]

On Mon, 2018-02-26 at 15:50 +0100, Volker Lendecke wrote:
> On Mon, Feb 26, 2018 at 02:33:54PM +0100, Ralph Böhme via samba-
> technical wrote:
> > Hi Swen,
> > 
> > On Mon, Feb 26, 2018 at 02:10:28PM +0100, Swen Schillig via samba-
> > technical wrote:
> > > A second review would be highly appreciated on the below.
> > > 
> > > I added the patch file for convenience.
> > > 
> > > Thanks in advance.
> > 
> > thanks for the cleanup, but I have one nitpick.
> > 
> > Why do you use unlikely() in an error check of a code path that is
> > probably not
> > performance critical?
> > 
> > If others don't convince me that this is good practice: nack,
> > sorry.
> 
> Same here: Swen, have you been able to measure a performance
> difference? And if so, what was your benchmark? I would be HIGHLY
> interested in seeing you how were able to actually measure this.

I didn't measure anything for those two "unlikely".
Actually, I didn't even think of having to prove under which
circumstances a gain would be visible.
My only goal was to give the good-path a slight little benefit over the
error-path, but yeah, that "benefit" might not even be measurable or
vanish in noise.

Besides, the reason of this patch wasn't the addition of the unlikely
but the fix of the mem-leak by using NT_STATUS_HAVE_NO_MEMORY.

So, if the "unlikely" is the only reason preventing this patch to get
pushed, I'll be more than happy to remove it.

> 
> In talloc, this *is* measurable, at least Tridge did some good
> micro-benchmarks showing some real improvement. Can you please
> document what you did to get real improvement in this piece of code
> to
> justify cluttering the code?

...cluttering the code ? Boah Volker, you're being a bit hard here,
don't you think :-)

Anyway, as I said above, I have no strong feelings about using or not
using "unlikely" here. I f the two of you prefer, I will remove it.

Cheers Swen