[PATCH 1.2] Clean up netgroupcache

[Ondřej Bílka]

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.

Also it is not clear at all that your branch is unlikely. If function
could only fail from insufficient buffer size then this check will be always 
be taken.

A simplified code flow is here:

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) [1]
  snip;
else 
  {
    if (status == NSS_STATUS_UNAVAIL && e == ERANGE) [2]
      snip;
  }