Re: [PATCH 1/25] Remove nested functions: crypt/md5-crypt.c

[Konstantin Serebryany <konstantin dot s dot serebryany at gmail dot com>]

On Tue, May 20, 2014 at 6:46 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Tue, May 20, 2014 at 06:23:40PM +0400, Konstantin Serebryany wrote:
>> 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.
>
> You'll need to choose inputs so that __md5_crypt_r is called -
> crypt/crypt-entry.c should help you with that.

With salt == "$1$" I get this profile:
 84.70%  a.out  libcrypt-2.15.so   [.] __md5_process_block
  5.98%  a.out  libc-2.15.so       [.] __memcpy_ssse3_back
  4.71%  a.out  libcrypt-2.15.so   [.] __md5_process_bytes
  1.92%  a.out  libcrypt-2.15.so   [.] __md5_crypt_r
  1.62%  a.out  libcrypt-2.15.so   [.] __md5_finish_ctx

Again, __md5_crypt_r is within noise.

>> Another thing that can be done is to look at the code generated for the
>> relevant file before and after the patch - I'd expect very little change.

I like this method much more, but there is actually a change.
The original binary has calls to b64_from_24bit.7858 (the nested function),
while in the binary built with my patch these calls are inlined.
Of course, this is a feature of a particular version of GCC, not of
the glibc code.


>   if __md5_crypt_r takes
> a tiny fraction of time again, then this change should be safe, but I
> would prefer that the inputs get fed into the benchtests anyway so
> that we can automatically track performance of crypt for those inputs
> in future.
>
> Siddhesh