[1.2] Clean up netgroupcache

  On Fri, Mar 28, 2014 at 08:17:37AM +0530, Siddhesh Poyarekar wrote:
> On Thu, Mar 27, 2014 at 10:15:16PM +0100, Ondřej Bílka wrote:
> > Had typo there, here is fixed patch.
> 
> Please add a note on how you tested the fix.
> 
> > 	* nscd/netgroupcache.c: Clean up implementation.
> 
> Incorrect ChangeLog entry.
> 
New one below.

> > -				|| nhost > nuser || nuser > ndomain)
> > +			    size_t newlen = buffilled + hostlen + userlen + domainlen;
> > +			    if (newlen + req->key_len > buflen)
> 
> Might as well put req->key_len in newlen and simplify this further.
>
ok 

> > @@ -328,8 +289,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
> >  		      }
> >  		    else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
> >  		      {
> > -			buflen *= 2;
> > -			buffer = xrealloc (buffer, buflen);
> > +			tempbuflen *= 2;
> > +			tempbuf = xrealloc (tempbuf, tempbuflen);
> 
> This ought to be unlikely now, since the consolidated lengths of user,
> host and domain should fit in the initially allocated 1024 bytes.
> 
I made change that previous branch is likely. I am not sure about this
one because its conditional probability. This condition is checked only
on failure. So likeliness is determined by probability that failure was
caused by insufficient size.


New version is here.


	* nscd/netgroupcache.c (addgetnetgrentX): Use separate buffer
	for result and temporary data.
  

On Fri, Apr 11, 2014 at 04:30:00PM +0200, Ondřej Bílka wrote:
> New version is here.
> 
> 
> 	* nscd/netgroupcache.c (addgetnetgrentX): Use separate buffer
> 	for result and temporary data.
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 820d823..9582ffb 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -138,8 +138,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>    char *key_copy = NULL;
>    struct __netgrent data;
>    size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len);
> +  size_t tempbuflen = 1024;
>    size_t buffilled = sizeof (*dataset);
>    char *buffer = NULL;
> +  char *tempbuf;
>    size_t nentries = 0;
>    size_t group_len = strlen (key) + 1;
>    union
> @@ -159,6 +161,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  
>    memset (&data, '\0', sizeof (data));
>    buffer = xmalloc (buflen);
> +  tempbuf = xmalloc (tempbuflen);
> +
>    first_needed.elem.next = &first_needed.elem;
>    memcpy (first_needed.elem.name, key, group_len);
>    data.needed_groups = &first_needed.elem;
> @@ -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.

>  		      {
>  			if (data.type == triple_val)
>  			  {
> @@ -220,53 +223,13 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  			    size_t userlen = strlen (nuser ?: "") + 1;
>  			    size_t domainlen = strlen (ndomain ?: "") + 1;
>  
> -			    if (nhost == NULL || nuser == NULL || ndomain == NULL
> -				|| nhost > nuser || nuser > ndomain)
> -			      {
> -				const char *last = nhost;
> -				if (last == NULL
> -				    || (nuser != NULL && nuser > last))
> -				  last = nuser;
> -				if (last == NULL
> -				    || (ndomain != NULL && ndomain > last))
> -				  last = ndomain;
> -
> -				size_t bufused
> -				  = (last == NULL
> -				     ? buffilled
> -				     : last + strlen (last) + 1 - buffer);
> -
> -				/* We have to make temporary copies.  */
> -				size_t needed = hostlen + userlen + domainlen;
> -
> -				if (buflen - req->key_len - bufused < needed)
> -				  {
> -				    buflen += MAX (buflen, 2 * needed);
> -				    /* Save offset in the old buffer.  We don't
> -				       bother with the NULL check here since
> -				       we'll do that later anyway.  */
> -				    size_t nhostdiff = nhost - buffer;
> -				    size_t nuserdiff = nuser - buffer;
> -				    size_t ndomaindiff = ndomain - buffer;
> -
> -				    char *newbuf = xrealloc (buffer, buflen);
> -				    /* Fix up the triplet pointers into the new
> -				       buffer.  */
> -				    nhost = (nhost ? newbuf + nhostdiff
> -					     : NULL);
> -				    nuser = (nuser ? newbuf + nuserdiff
> -					     : NULL);
> -				    ndomain = (ndomain ? newbuf + ndomaindiff
> -					       : NULL);
> -				    buffer = newbuf;
> -				  }
> +			    size_t newlen = buffilled + hostlen + userlen
> +					  + domainlen + req->key_len;
>  
> -				nhost = memcpy (buffer + bufused,
> -						nhost ?: "", hostlen);
> -				nuser = memcpy ((char *) nhost + hostlen,
> -						nuser ?: "", userlen);
> -				ndomain = memcpy ((char *) nuser + userlen,
> -						  ndomain ?: "", domainlen);
> +			    if (__glibc_unlikely (newlen > buflen))
> +			      {
> +				buflen = 2 * newlen;
> +				buffer = xrealloc (buffer, buflen);
>  			      }
>  
>  			    char *wp = buffer + buffilled;
> @@ -328,8 +291,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  		      }
>  		    else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
>  		      {
> -			buflen *= 2;
> -			buffer = xrealloc (buffer, buflen);
> +			tempbuflen *= 2;
> +			tempbuf = xrealloc (tempbuf, tempbuflen);
>  		      }
>  		  }
>  
> @@ -474,6 +437,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>      }
>  
>   out:
> +  free (tempbuf);
>    free (buffer);
>  
>    *resultp = dataset;

The patch looks OK otherwise.  However, I'd like to know your feedback
from actually testing this patch.

Thanks,
Siddhesh
  

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;
  }
  

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