Message ID | 20140411143000.GA30142@domone.podge |
---|---|
State | New |
Headers | show |
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
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)) { 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;