[PATCH 1.2] Clean up netgroupcache

[Siddhesh Poyarekar]

On Mon, Apr 28, 2014 at 01:43:59PM +0200, Ondřej Bílka wrote:
> On Fri, Apr 11, 2014 at 10:33:56PM +0530, Siddhesh Poyarekar wrote:
> > On Fri, Apr 11, 2014 at 04:30:00PM +0200, Ondřej Bílka wrote:
> > > @@ -201,14 +205,13 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
> > >  		while (1)
> > >  		  {
> > >  		    int e;
> > > -		    status = getfct.f (&data, buffer + buffilled,
> > > -				       buflen - buffilled - req->key_len, &e);
> > > +		    status = getfct.f (&data, tempbuf, tempbuflen, &e);
> > >  		    if (status == NSS_STATUS_RETURN
> > >  			|| status == NSS_STATUS_NOTFOUND)
> > >  		      /* This was either the last one for this group or the
> > >  			 group was empty.  Look at next group if available.  */
> > >  		      break;
> > > -		    if (status == NSS_STATUS_SUCCESS)
> > > +		    if (__glibc_likely (status == NSS_STATUS_SUCCESS))
> > 
> > This is an unrelated change.  It could be proposed as a separate
> > change, but I'm not convinced that it is necessary or even useful
> > unlike the glibc_unlikely below, which is unlikely because of the fact
> > that host, user and domain sizes should not normally exceed 1024 bytes
> > given their defined limitations.
> 
> See my previous comment. It is the change you need to make to actually
> improve performance. You need to analyze code flow (below) more
> carefully. My check [1] happens before the check you proposed [2]. And
> precisely because range condition is rare your unlikely the branch with
> you check will be in 99% dead thus where improving performance is completely
> unnecessary. In general you should not try to optimize for error conditions.

You have not contradicted my point that the change is unrelated to the
current patch, so please split it out and post it as a separate
change.  I agree that I wasn't convinced that the annotation was
necessary, but I won't discuss that here since it would only serve to
derail discussion on the rest of the patch, which is largely valid.
You only need to post detailed results of testing your change and the
rest of the patch is good to go.

Siddhesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20140428/dd9ca0ec/attachment.sig>