This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1.2] Clean up netgroupcache
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: Mike Frysinger <vapier at gentoo dot org>, libc-alpha at sourceware dot org
- Date: Mon, 28 Apr 2014 13:43:59 +0200
- Subject: Re: [PATCH 1.2] Clean up netgroupcache
- Authentication-results: sourceware.org; auth=none
- References: <20140327040406 dot GA26264 at spoyarek dot pnq dot redhat dot com> <1499542 dot yzGAIksTkn at vapier> <20140327192254 dot GC1982 at domone dot podge> <20140327203207 dot GA2476 at domone dot podge> <20140327211516 dot GB2476 at domone dot podge> <20140328024737 dot GJ31211 at spoyarek dot pnq dot redhat dot com> <20140411143000 dot GA30142 at domone dot podge> <20140411170356 dot GN25518 at spoyarek dot pnq dot redhat dot com>
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;
}