From patchwork Tue May 20 14:23:40 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kostya Serebryany X-Patchwork-Id: 1031 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (peon2454.g.dreamhost.com [208.113.200.127]) by wilcox.dreamhost.com (Postfix) with ESMTP id 0D724360076 for ; Tue, 20 May 2014 07:24:07 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14307373) id B02F063FE90C2; Tue, 20 May 2014 07:24:07 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 97CED63FE90B5 for ; Tue, 20 May 2014 07:24:07 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; q=dns; s=default; b=qrh9 3vEgFfDWWprr1hNaCRojhfVUHwMEAr8wkldRwjRG09Ye67wbBZr28/ASLQNLRzDZ tcZeeuxjRxif993rgc852I0AVwsLFVF3vA+3K8cUuB6KtHKM4SyjRfNN58D5a0gx hk9aJWLejkc6RLwkxODMK1KC2KDZLigPlEc4uBU= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; s=default; bh=tHFJF6Ytqh QkAWY7mYIczb4lutM=; b=GxnjTU6DiQXcMPzN9Hp24RRXdXYgxzjE798dkg2mq8 c5ANGhceyI6A9/vBks62pqV0xctCCjebyAys88PgDCvnk7nDopVKh75KBt029cNr 2Oh9KVpvI1PLZSdNVrD6pkKx69WX8vsZh/CiJH75o165ZytByzB/TGd6PgbCkqa0 0= Received: (qmail 3563 invoked by alias); 20 May 2014 14:24:06 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 3549 invoked by uid 89); 20 May 2014 14:24:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ve0-f169.google.com X-Received: by 10.58.188.115 with SMTP id fz19mr1965055vec.40.1400595840910; Tue, 20 May 2014 07:24:00 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20140520131314.GB14500@spoyarek.pnq.redhat.com> References: <20140520131314.GB14500@spoyarek.pnq.redhat.com> From: Konstantin Serebryany Date: Tue, 20 May 2014 18:23:40 +0400 Message-ID: Subject: Re: [PATCH 1/25] Remove nested functions: crypt/md5-crypt.c To: Siddhesh Poyarekar Cc: GNU C Library X-DH-Original-To: glibc@patchwork.siddhesh.in 2014-05-20 Kostya Serebryany * crypt/md5-crypt.c (__md5_crypt_r): Remove a nested function. (b64_from_24bit): New function. On Tue, May 20, 2014 at 5:13 PM, Siddhesh Poyarekar wrote: > On Tue, May 20, 2014 at 04:56:27PM +0400, Konstantin Serebryany wrote: >> Hi, >> >> This patch is the first in the series of patches that remove nested >> functions from glibc. >> Rationale: nested functions is a non-standard language feature; >> removing nested functions >> will allow to compile glibc with compilers other than GCC and thus >> benefit from other compilers >> and code analysis tools. >> Discussed previously here: >> https://sourceware.org/ml/libc-alpha/2014-05/msg00400.html >> No functionality change intended. >> >> (If approved, this will be my first commit to glibc, so some more >> actions may be required) > > I believe initially you'll need one of us to commit for you, which we > will do once the patch is found suitable. You may want to review the > contribution checklist as well: > > http://sourceware.org/glibc/wiki/Contribution%20checklist > >> >> 2014-05-20 Kostya Serebryany > > Two spaces between the date and name, and name and email. An extra > newline between the ChangeLog content and header. > >> * crypt/md5-crypt.c (__md5_crypt_r): Remove a nested function. >> (b64_from_24bit): New function. >> >> >> diff --git a/crypt/md5-crypt.c b/crypt/md5-crypt.c >> index d1b92d7..29f66ce 100644 >> --- a/crypt/md5-crypt.c >> +++ b/crypt/md5-crypt.c >> @@ -88,6 +88,19 @@ extern char *__md5_crypt_r (const char *key, const >> char *salt, >> char *buffer, int buflen); >> extern char *__md5_crypt (const char *key, const char *salt); >> >> +static void b64_from_24bit (char **cp, int *buflen, > > 'static void' should be on a separate line on its own. done > >> + unsigned int b2, unsigned int b1, unsigned int b0, >> + int n) >> +{ >> + unsigned int w = (b2 << 16) | (b1 << 8) | b0; >> + while (n-- > 0 && *buflen > 0) >> + { >> + *(*cp)++ = b64t[w & 0x3f]; >> + --*buflen; >> + w >>= 6; >> + } >> +} >> + >> >> /* This entry point is equivalent to the `crypt' function in Unix >> libcs. */ >> @@ -268,24 +281,18 @@ __md5_crypt_r (key, salt, buffer, buflen) >> --buflen; >> } >> >> - void b64_from_24bit (unsigned int b2, unsigned int b1, unsigned int b0, >> - int n) >> - { >> - unsigned int w = (b2 << 16) | (b1 << 8) | b0; >> - while (n-- > 0 && buflen > 0) >> - { >> - *cp++ = b64t[w & 0x3f]; >> - --buflen; >> - w >>= 6; >> - } >> - } >> - >> - b64_from_24bit (alt_result[0], alt_result[6], alt_result[12], 4); >> - b64_from_24bit (alt_result[1], alt_result[7], alt_result[13], 4); >> - b64_from_24bit (alt_result[2], alt_result[8], alt_result[14], 4); >> - b64_from_24bit (alt_result[3], alt_result[9], alt_result[15], 4); >> - b64_from_24bit (alt_result[4], alt_result[10], alt_result[5], 4); >> - b64_from_24bit (0, 0, alt_result[11], 2); >> + b64_from_24bit (&cp, &buflen, >> + alt_result[0], alt_result[6], alt_result[12], 4); >> + b64_from_24bit (&cp, &buflen, >> + alt_result[1], alt_result[7], alt_result[13], 4); >> + b64_from_24bit (&cp, &buflen, >> + alt_result[2], alt_result[8], alt_result[14], 4); >> + b64_from_24bit (&cp, &buflen, >> + alt_result[3], alt_result[9], alt_result[15], 4); >> + b64_from_24bit (&cp, &buflen, >> + alt_result[4], alt_result[10], alt_result[5], 4); >> + b64_from_24bit (&cp, &buflen, >> + 0, 0, alt_result[11], 2); >> if (buflen <= 0) >> { >> __set_errno (ERANGE); > > It would be useful if you could post some kind of performance analysis > of the crypt or crypt_r to show that making this change does not cause > a significant drop in performance. In fact, I'd be really interested > in having a microbenchmark added for this function in benchtests. > Instructions in benchtests/README should be useful but if not, please > feel free to reach out to me for help. Do you think such a benchmarks is possible at all? I've made a simple test (attached) and profiled it with "perf": 85.26% a.out libcrypt-2.15.so [.] _ufc_doit_r 6.13% a.out libcrypt-2.15.so [.] _ufc_mk_keytab_r 2.40% a.out libcrypt-2.15.so [.] _ufc_setup_salt_r 1.56% a.out libcrypt-2.15.so [.] __crypt_r 1.51% a.out libcrypt-2.15.so [.] _ufc_output_conversion_r 1.35% a.out libcrypt-2.15.so [.] crypt As you can see, crypt_r itself takes a tiny fraction of time, most of it is spent in _ufc_* which are defined in another module. Any changes in crypt itself (unless you do something insane) will not be observable in profile. Updated patch: libcs. */ @@ -268,24 +282,18 @@ __md5_crypt_r (key, salt, buffer, buflen) --buflen; } - void b64_from_24bit (unsigned int b2, unsigned int b1, unsigned int b0, - int n) - { - unsigned int w = (b2 << 16) | (b1 << 8) | b0; - while (n-- > 0 && buflen > 0) - { - *cp++ = b64t[w & 0x3f]; - --buflen; - w >>= 6; - } - } - - b64_from_24bit (alt_result[0], alt_result[6], alt_result[12], 4); - b64_from_24bit (alt_result[1], alt_result[7], alt_result[13], 4); - b64_from_24bit (alt_result[2], alt_result[8], alt_result[14], 4); - b64_from_24bit (alt_result[3], alt_result[9], alt_result[15], 4); - b64_from_24bit (alt_result[4], alt_result[10], alt_result[5], 4); - b64_from_24bit (0, 0, alt_result[11], 2); + b64_from_24bit (&cp, &buflen, + alt_result[0], alt_result[6], alt_result[12], 4); + b64_from_24bit (&cp, &buflen, + alt_result[1], alt_result[7], alt_result[13], 4); + b64_from_24bit (&cp, &buflen, + alt_result[2], alt_result[8], alt_result[14], 4); + b64_from_24bit (&cp, &buflen, + alt_result[3], alt_result[9], alt_result[15], 4); + b64_from_24bit (&cp, &buflen, + alt_result[4], alt_result[10], alt_result[5], 4); + b64_from_24bit (&cp, &buflen, + 0, 0, alt_result[11], 2); if (buflen <= 0) { __set_errno (ERANGE); diff --git a/crypt/md5-crypt.c b/crypt/md5-crypt.c index d1b92d7..5d5fc55 100644 --- a/crypt/md5-crypt.c +++ b/crypt/md5-crypt.c @@ -88,6 +88,20 @@ extern char *__md5_crypt_r (const char *key, const char *salt, char *buffer, int buflen); extern char *__md5_crypt (const char *key, const char *salt); +static void +b64_from_24bit (char **cp, int *buflen, + unsigned int b2, unsigned int b1, unsigned int b0, + int n) +{ + unsigned int w = (b2 << 16) | (b1 << 8) | b0; + while (n-- > 0 && *buflen > 0) + { + *(*cp)++ = b64t[w & 0x3f]; + --*buflen; + w >>= 6; + } +} + /* This entry point is equivalent to the `crypt' function in Unix