This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [PATCH/RFA]: Optimize wctomb/mbtowc functions for speed
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.
> 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?
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.
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;
--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat