This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH/RFA]: Optimize wctomb/mbtowc functions for speed

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;



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]