[PATCH/RFA]: Optimize wctomb/mbtowc functions for speed
Jeff Johnston
jjohnstn@redhat.com
Wed Nov 18 10:42:00 GMT 2009
On 17/11/09 04:38 AM, Corinna Vinschen wrote:
> On Nov 13 20:23, Jeff Johnston wrote:
>> On 06/11/09 02:55 PM, Corinna Vinschen wrote:
>>> const char in[] = "The quick brown fox jumps over the lazy dog";
>>> for (count = 0; count< 1000000; ++count)
>>> for (i = 0; i< size; i += mbclen)
>>> if ((int)(mbclen = mbrtowc(&wc, in + i, size - i,&mbs))<= 0)
>>> break;
>>>
>>> is about 45% faster
>>> [...]
>>> Additionally, the functions mbrtowc and wcrtomb are now implemented
>>> independently from _mbrtowc_r and _wcrtomb_r, unless
>>> PREFER_SIZE_OVER_SPEED is defined.
>>
>> I'm not in agreement with the last part of this patch which is to
>> add a PREFER_SIZE_OVER_SPEED check for mbrtowc and wcrtomb. Where
>> does one draw the line? Should the set of I/O functions that printf
>> and scanf call be conglomerated into one massive function unless
>> asked not to? It would save time.
>
> No. Not really. There's a big difference between a printf call and a
> mbrtowc call. The printf call is typically a long running call anyway,
> since it operates on strings and performs a complex task on these strings.
>
> mbrtowc and wcrtomb on the other hand are called in loops from the
> application for every single character in a string. Single character
> functions should be as fast as possible, otherwise they are waisting
> proportionally much more time than a printf function ever could.
>
>> I would
>> guess that this one call isn't giving you that much improvement on
>> average compared to the optimization you made with the lower-level
>> calls
>
> Take the above example code which is somewhat artificial, but is a
> fairly good approximation of what happens in grep with LANG=en_US.UTF-8.
>
> Assume you built newlib with -g -O2 on x86, using gcc 4.3.4. I ran the
> above example with a count of 2.000.000. Here's the result on a 2.8 GHz
> Opteron machine, with all optimizations from my patch, except for the
> PREFER_SIZE_OVER_SPEED change:
>
> $ for i in $(seq 1 10); do (time ./mb) 2>&1 | grep user; done
> user 0m2.484s
> user 0m2.593s
> user 0m2.593s
> user 0m2.671s
> user 0m2.687s
> user 0m2.749s
> user 0m2.765s
> user 0m2.687s
> user 0m2.749s
> user 0m2.750s
>
> Average: 2.673s
>
> And now the timing for the same scenario, with the PREFER_SIZE_OVER_SPEED
> change in mbrtowc:
>
> $ for i in $(seq 1 10); do (time ./mb) 2>&1 | grep user; done
> user 0m1.859s
> user 0m1.921s
> user 0m1.859s
> user 0m2.000s
> user 0m1.984s
> user 0m1.937s
> user 0m1.734s
> user 0m1.937s
> user 0m1.921s
> user 0m1.796s
>
> Average: 1.895s
>
> That's 29% faster, just due to the missing intermediate call to _mbrtowc_r.
> So, sorry, but I can't agree with you. This one call does make a huge
> difference on average.
>
Can't argue with the figures and yes, this function does get used a lot
for applications that are forced to support wide chars.
>> and also, Cygwin could use a macro trick itself to call all
>> the _r function(s) directly and save time.
>
> It's not Cygwin I'm talking about, it's applications. Cygwin has
> nothing to do with it, except when using some character sets like GBK or
> Big5 on the lowest level of the call chain. You don't really mean to
> change these function calls to macros on the application level, right?
>
Correct.
> And I'm not talking of changing every single non-reentrant function that
> way. But functions like mbrtowc and wcrtomb are something special.
> They are single character functions, most of the time called in loops
> from the application. Single character functions waisting so much time
> are not really desired, IMHO. If we find other single character
> functions which waste so much time, I would vote to change them as well
> at one point.
>
>> Now, that said, I totally agree with the heart of the change which
>> is to replace the calls to _mbtowc_r and _wctomb_r as they are no
>> longer performing any real logic.
>>
>> I think it would be better to call the functions directly instead of
>> hiding them behind the macros. The effort to do this is just a sed
>> operation and the macro just gets in the way for debugging and
>> understanding the code. It is not a temporary change or one that is
>> governed by a switch which would more justify a macro solution.
>
> Here's the patch which calls the __mbtowc and __wctomb functions
> directly. It also leaves out the PREFER_SIZE_OVER_SPEED improvement, so
> I guess it should be ok to go in. However, I would really like yoy to
> reconsider the PREFER_SIZE_OVER_SPEED change. It really makes a big
> difference from the application POV.
>
Ok, I am convinced. Feel free to check in the modified patch.
-- Jeff J.
>
> Thanks,
> Corinna
>
> * libc/stdio/vfprintf.c: Include ../stdlib/local.h. Replace call to
> _mbtowc_r with direct call to __mbtowc.
> * libc/stdio/vfscanf.c: Ditto.
> * libc/stdlib/btowc.c: Include local.h. Replace call to _mbtowc_r
> with direct call to __mbtowc.
> * libc/stdlib/mblen.c: Ditto.
> * libc/stdlib/mblen_r.c: Ditto.
> * libc/stdlib/mbrtowc.c: Ditto.
> * libc/stdlib/mbstowcs_r.c: Ditto.
> * libc/stdlib/mbtowc.c: Ditto.
> * libc/stdlib/wcrtomb.c: Include local.h. Replace call to _wctomb_r
> with direct call to __wctomb.
> * libc/stdlib/wcsnrtombs.c: Ditto.
> (_wcsnrtombs_r): Ditto.
> * libc/stdlib/wcstombs_r.c: Ditto.
> * libc/stdlib/wctob.c: Ditto.
> * libc/stdlib/wctomb.c: Ditto.
>
> * libc/stdlib/mbtowc_r.c (__utf8_mbtowc): Drop unnecessary test for
> ch>= 0.
>
>
> Index: libc/stdio/vfprintf.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/vfprintf.c,v
> retrieving revision 1.75
> diff -u -p -r1.75 vfprintf.c
> --- libc/stdio/vfprintf.c 16 Jun 2009 17:44:20 -0000 1.75
> +++ libc/stdio/vfprintf.c 17 Nov 2009 09:35:59 -0000
> @@ -159,6 +159,7 @@ static char *rcsid = "$Id: vfprintf.c,v
> #include<sys/lock.h>
> #include<stdarg.h>
> #include "local.h"
> +#include "../stdlib/local.h"
> #include "fvwrite.h"
> #include "vfieeefp.h"
>
> @@ -722,7 +723,8 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap)
> for (;;) {
> cp = fmt;
> #ifdef _MB_CAPABLE
> - while ((n = _mbtowc_r (data,&wc, fmt, MB_CUR_MAX,&state))> 0) {
> + while ((n = __mbtowc (data,&wc, fmt, MB_CUR_MAX,
> + __locale_charset (),&state))> 0) {
> if (wc == '%')
> break;
> fmt += n;
> @@ -1794,7 +1796,8 @@ _DEFUN(get_arg, (data, n, fmt, ap, numar
> while (*fmt&& n>= numargs)
> {
> # ifdef _MB_CAPABLE
> - while ((nbytes = _mbtowc_r (data,&wc, fmt, MB_CUR_MAX,&wc_state))> 0)
> + while ((nbytes = __mbtowc (data,&wc, fmt, MB_CUR_MAX,
> + __locale_charset (),&wc_state))> 0)
> {
> fmt += nbytes;
> if (wc == '%')
> Index: libc/stdio/vfscanf.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/vfscanf.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 vfscanf.c
> --- libc/stdio/vfscanf.c 11 Mar 2009 11:53:22 -0000 1.47
> +++ libc/stdio/vfscanf.c 17 Nov 2009 09:36:00 -0000
> @@ -122,6 +122,7 @@ Supporting OS subroutines required:
> #include<stdarg.h>
> #include<errno.h>
> #include "local.h"
> +#include "../stdlib/local.h"
>
> #ifdef INTEGER_ONLY
> #define VFSCANF vfiscanf
> @@ -506,7 +507,8 @@ _DEFUN(__SVFSCANF_R, (rptr, fp, fmt0, ap
> wc = *fmt;
> #else
> memset (&state, '\0', sizeof (state));
> - nbytes = _mbtowc_r (rptr,&wc, fmt, MB_CUR_MAX,&state);
> + nbytes = __mbtowc (rptr,&wc, fmt, MB_CUR_MAX, __locale_charset (),
> + &state);
> #endif
> fmt += nbytes;
> if (wc == 0)
> Index: libc/stdlib/btowc.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/btowc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 btowc.c
> --- libc/stdlib/btowc.c 29 May 2007 21:26:59 -0000 1.2
> +++ libc/stdlib/btowc.c 17 Nov 2009 09:36:00 -0000
> @@ -3,6 +3,7 @@
> #include<stdio.h>
> #include<reent.h>
> #include<string.h>
> +#include "local.h"
>
> wint_t
> btowc (int c)
> @@ -19,7 +20,7 @@ btowc (int c)
>
> _REENT_CHECK_MISC(_REENT);
>
> - retval = _mbtowc_r (_REENT,&pwc,&b, 1,&mbs);
> + retval = __mbtowc (_REENT,&pwc,&b, 1, __locale_charset (),&mbs);
>
> if (c == EOF || retval != 1)
> return WEOF;
> Index: libc/stdlib/mblen.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mblen.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 mblen.c
> --- libc/stdlib/mblen.c 23 Apr 2004 21:44:22 -0000 1.5
> +++ libc/stdlib/mblen.c 17 Nov 2009 09:36:00 -0000
> @@ -46,6 +46,7 @@ effects vary with the locale.
> #include<newlib.h>
> #include<stdlib.h>
> #include<wchar.h>
> +#include "local.h"
>
> int
> _DEFUN (mblen, (s, n),
> @@ -58,7 +59,7 @@ _DEFUN (mblen, (s, n),
>
> _REENT_CHECK_MISC(_REENT);
> state =&(_REENT_MBLEN_STATE(_REENT));
> - retval = _mbtowc_r (_REENT, NULL, s, n, state);
> + retval = __mbtowc (_REENT, NULL, s, n, __locale_charset (), state);
> if (retval< 0)
> {
> state->__count = 0;
> Index: libc/stdlib/mblen_r.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mblen_r.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 mblen_r.c
> --- libc/stdlib/mblen_r.c 23 Apr 2004 21:44:22 -0000 1.4
> +++ libc/stdlib/mblen_r.c 17 Nov 2009 09:36:00 -0000
> @@ -46,6 +46,7 @@ effects vary with the locale.
> #include<newlib.h>
> #include<stdlib.h>
> #include<wchar.h>
> +#include "local.h"
>
> int
> _DEFUN (_mblen_r, (r, s, n, state),
> @@ -56,7 +57,7 @@ _DEFUN (_mblen_r, (r, s, n, state),
> {
> #ifdef _MB_CAPABLE
> int retval;
> - retval = _mbtowc_r (r, NULL, s, n, state);
> + retval = __mbtowc (r, NULL, s, n, __locale_charset (), state);
>
> if (retval< 0)
> {
> Index: libc/stdlib/mbrtowc.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mbrtowc.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 mbrtowc.c
> --- libc/stdlib/mbrtowc.c 23 Apr 2004 21:44:22 -0000 1.5
> +++ libc/stdlib/mbrtowc.c 17 Nov 2009 09:36:00 -0000
> @@ -5,6 +5,7 @@
> #include<stdio.h>
> #include<errno.h>
> #include<string.h>
> +#include "local.h"
>
> size_t
> _DEFUN (_mbrtowc_r, (ptr, pwc, s, n, ps),
> @@ -25,9 +26,9 @@ _DEFUN (_mbrtowc_r, (ptr, pwc, s, n, ps)
> #endif
>
> if (s == NULL)
> - retval = _mbtowc_r (ptr, NULL, "", 1, ps);
> + retval = __mbtowc (ptr, NULL, "", 1, __locale_charset (), ps);
> else
> - retval = _mbtowc_r (ptr, pwc, s, n, ps);
> + retval = __mbtowc (ptr, pwc, s, n, __locale_charset (), ps);
>
> if (retval == -1)
> {
> Index: libc/stdlib/mbstowcs_r.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mbstowcs_r.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 mbstowcs_r.c
> --- libc/stdlib/mbstowcs_r.c 17 Mar 2009 12:16:28 -0000 1.4
> +++ libc/stdlib/mbstowcs_r.c 17 Nov 2009 09:36:00 -0000
> @@ -1,5 +1,6 @@
> #include<stdlib.h>
> #include<wchar.h>
> +#include "local.h"
>
> size_t
> _DEFUN (_mbstowcs_r, (reent, pwcs, s, n, state),
> @@ -17,7 +18,7 @@ _DEFUN (_mbstowcs_r, (reent, pwcs, s, n,
> n = (size_t) 1; /* Value doesn't matter as long as it's not 0. */
> while (n> 0)
> {
> - bytes = _mbtowc_r (r, pwcs, t, MB_CUR_MAX, state);
> + bytes = __mbtowc (r, pwcs, t, MB_CUR_MAX, __locale_charset (), state);
> if (bytes< 0)
> {
> state->__count = 0;
> Index: libc/stdlib/mbtowc.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mbtowc.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 mbtowc.c
> --- libc/stdlib/mbtowc.c 23 Apr 2004 21:44:22 -0000 1.5
> +++ libc/stdlib/mbtowc.c 17 Nov 2009 09:36:00 -0000
> @@ -54,6 +54,7 @@ effects vary with the locale.
> #include<newlib.h>
> #include<stdlib.h>
> #include<wchar.h>
> +#include "local.h"
>
> int
> _DEFUN (mbtowc, (pwc, s, n),
> @@ -68,7 +69,7 @@ _DEFUN (mbtowc, (pwc, s, n),
> _REENT_CHECK_MISC(_REENT);
> ps =&(_REENT_MBTOWC_STATE(_REENT));
>
> - retval = _mbtowc_r (_REENT, pwc, s, n, ps);
> + retval = __mbtowc (_REENT, pwc, s, n, __locale_charset (), ps);
>
> if (retval< 0)
> {
> Index: libc/stdlib/mbtowc_r.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/mbtowc_r.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 mbtowc_r.c
> --- libc/stdlib/mbtowc_r.c 3 Oct 2009 08:51:07 -0000 1.17
> +++ libc/stdlib/mbtowc_r.c 17 Nov 2009 09:36:00 -0000
> @@ -221,7 +221,7 @@ _DEFUN (__utf8_mbtowc, (r, pwc, s, n, ch
> return 0; /* s points to the null character */
> }
>
> - if (ch>= 0x0&& ch<= 0x7f)
> + if (ch<= 0x7f)
> {
> /* single-byte sequence */
> state->__count = 0;
> Index: libc/stdlib/wcrtomb.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/wcrtomb.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 wcrtomb.c
> --- libc/stdlib/wcrtomb.c 23 Apr 2004 21:44:22 -0000 1.4
> +++ libc/stdlib/wcrtomb.c 17 Nov 2009 09:36:00 -0000
> @@ -4,6 +4,7 @@
> #include<stdlib.h>
> #include<stdio.h>
> #include<errno.h>
> +#include "local.h"
>
> size_t
> _DEFUN (_wcrtomb_r, (ptr, s, wc, ps),
> @@ -24,9 +25,9 @@ _DEFUN (_wcrtomb_r, (ptr, s, wc, ps),
> #endif
>
> if (s == NULL)
> - retval = _wctomb_r (ptr, buf, L'\0', ps);
> + retval = __wctomb (ptr, buf, L'\0', __locale_charset (), ps);
> else
> - retval = _wctomb_r (ptr, s, wc, ps);
> + retval = __wctomb (ptr, s, wc, __locale_charset (), ps);
>
> if (retval == -1)
> {
> Index: libc/stdlib/wcsnrtombs.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/wcsnrtombs.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 wcsnrtombs.c
> --- libc/stdlib/wcsnrtombs.c 19 Feb 2009 09:19:42 -0000 1.1
> +++ libc/stdlib/wcsnrtombs.c 17 Nov 2009 09:36:00 -0000
> @@ -99,6 +99,7 @@ PORTABILITY
> #include<stdlib.h>
> #include<stdio.h>
> #include<errno.h>
> +#include "local.h"
>
> size_t
> _DEFUN (_wcsnrtombs_r, (r, dst, src, nwc, len, ps),
> @@ -134,7 +135,7 @@ _DEFUN (_wcsnrtombs_r, (r, dst, src, nwc
> {
> int count = ps->__count;
> wint_t wch = ps->__value.__wch;
> - int bytes = _wcrtomb_r (r, buff, *pwcs, ps);
> + int bytes = __wctomb (r, buff, *pwcs, __locale_charset (), ps);
> if (bytes == -1)
> {
> r->_errno = EILSEQ;
> @@ -160,7 +161,7 @@ _DEFUN (_wcsnrtombs_r, (r, dst, src, nwc
> }
> else
> {
> - /* not enough room, we must back up state to before _wctomb_r call */
> + /* not enough room, we must back up state to before __wctomb call */
> ps->__count = count;
> ps->__value.__wch = wch;
> len = 0;
> Index: libc/stdlib/wcstombs_r.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/wcstombs_r.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 wcstombs_r.c
> --- libc/stdlib/wcstombs_r.c 23 Oct 2007 19:50:29 -0000 1.3
> +++ libc/stdlib/wcstombs_r.c 17 Nov 2009 09:36:00 -0000
> @@ -1,5 +1,6 @@
> #include<stdlib.h>
> #include<wchar.h>
> +#include "local.h"
>
> size_t
> _DEFUN (_wcstombs_r, (reent, s, pwcs, n, state),
> @@ -18,14 +19,14 @@ _DEFUN (_wcstombs_r, (reent, s, pwcs, n,
> {
> size_t num_bytes = 0;
> while (*pwcs != 0)
> - num_bytes += _wctomb_r (r, buff, *pwcs++, state);
> + num_bytes += __wctomb (r, buff, *pwcs++, __locale_charset (), state);
> return num_bytes;
> }
> else
> {
> while (n> 0)
> {
> - int bytes = _wctomb_r (r, buff, *pwcs, state);
> + int bytes = __wctomb (r, buff, *pwcs, __locale_charset (), state);
> if (bytes == -1)
> return -1;
> num_to_copy = (n> bytes ? bytes : (int)n);
> Index: libc/stdlib/wctob.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/wctob.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 wctob.c
> --- libc/stdlib/wctob.c 29 May 2007 21:26:59 -0000 1.3
> +++ libc/stdlib/wctob.c 17 Nov 2009 09:36:00 -0000
> @@ -3,6 +3,7 @@
> #include<stdlib.h>
> #include<stdio.h>
> #include<string.h>
> +#include "local.h"
>
> int
> wctob (wint_t c)
> @@ -16,7 +17,7 @@ wctob (wint_t c)
>
> _REENT_CHECK_MISC(_REENT);
>
> - retval = _wctomb_r (_REENT,&pwc, c,&mbs);
> + retval = __wctomb (_REENT,&pwc, c, __locale_charset (),&mbs);
>
> if (c == EOF || retval != 1)
> return WEOF;
> Index: libc/stdlib/wctomb.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdlib/wctomb.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 wctomb.c
> --- libc/stdlib/wctomb.c 2 Mar 2009 23:30:59 -0000 1.4
> +++ libc/stdlib/wctomb.c 17 Nov 2009 09:36:00 -0000
> @@ -49,6 +49,7 @@ effects vary with the locale.
> #include<newlib.h>
> #include<stdlib.h>
> #include<errno.h>
> +#include "local.h"
>
> int
> _DEFUN (wctomb, (s, wchar),
> @@ -58,7 +59,8 @@ _DEFUN (wctomb, (s, wchar),
> #ifdef _MB_CAPABLE
> _REENT_CHECK_MISC(_REENT);
>
> - return _wctomb_r (_REENT, s, wchar,&(_REENT_WCTOMB_STATE(_REENT)));
> + return __wctomb (_REENT, s, wchar, __locale_charset (),
> + &(_REENT_WCTOMB_STATE(_REENT)));
> #else /* not _MB_CAPABLE */
> if (s == NULL)
> return 0;
>
>
More information about the Newlib
mailing list