Re: [musl] [PATCH] improve strerror speed

[Timo Teras <timo.teras@iki.fi>]

On Wed, 4 Mar 2020 12:06:21 +0300 (MSK)
Alexander Monakov <amonakov@ispras.ru> wrote:

> Hi,
> 
> On Tue, 3 Mar 2020, Timo Teräs wrote:
> 
> > change the current O(n) lookup to O(1) based on the machinery
> > described in "How To Write Shared Libraries" (Appendix B).  
> 
> I'm curious about the background of this change, did the inefficiency
> came up in practice?

Yes, it's the openssl querying all possible strerrors:
https://github.com/openssl/openssl/blob/master/crypto/err/err.c#L181

That makes it up to valgrind --tool=callgrind to peak the strerror_l in
performance analysis on short running programs:

 673,622  /data/aports/main/musl/src/musl-1.1.24/src/errno/strerror.c:strerror_l [/lib/ld-musl-x86_64.so.1]

This change completely removes strerror from there.

> > --- a/src/errno/__strerror.h
> > +++ b/src/errno/__strerror.h
> > @@ -1,8 +1,9 @@
> > -/* This file is sorted such that 'errors' which represent
> > exceptional
> > - * conditions under which a correct program may fail come first,
> > followed
> > - * by messages that indicate an incorrect program or system
> > failure. The
> > - * macro E() along with double-inclusion is used to ensure that
> > ordering
> > - * of the strings remains synchronized. */
> > +/* The first '0' mapping will be used for error codes that
> > + * are not explicitly mentioned here.
> > + * This file is included multiple times to generate struct
> > + * populate it's content and create a fast lookup index to it. */  
> 
> The last sentence seems to have typos ("a struct,", "its").
> I would write the comment like this:
> 
> /* The first entry is a catch-all for codes not enumerated here.
>  * This file is included multiple times to declare and define a
> structure
>  * with messages, and then to define a lookup table translating error
> codes
>  * to offsets of corresponding fields in the structure. */
> 
> > +	if (e < 0 || e >= sizeof(errmsgidx)/sizeof(errmsgidx[0]))
> > e = 0;  
> 
> I think usually in musl such range checks are written in an
> easier-to-optimize form that tests the argument in an unsigned type,
> e.g. like this:
> 
>   if ((size_t)e >= sizeof errmsgidx / sizeof *errmsgidx) e = 0;

Thanks, will update and resend.

Timo