[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 4 Mar 2020 11:17:31 +0200
From: Timo Teras <timo.teras@....fi>
To: Alexander Monakov <amonakov@...ras.ru>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] improve strerror speed
On Wed, 4 Mar 2020 12:06:21 +0300 (MSK)
Alexander Monakov <amonakov@...ras.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
