[PATCH] Speed-up sprintf() family of functions.
Jonathan Larmour
jifl@eCosCentric.com
Fri Dec 29 14:44:00 GMT 2006
Sergei Organov wrote:
> Hello,
>
> The patch below boosts performance of sprintf() family of functions by a
> factor of 2 or more (measured on ARM7). Basically it gets rid of all the
> overhead of Cyg_StdioStream when printing to strings without introducing
> any new sprintf-specific code. Refer to ChangeLog below for some numbers.
It's a good thing which people may well want to have. However, we have
traditionally shied away from virtual functions in the past, as an
intentional design choice. Admittedly, that really dates from the time
when the C++ compiler was in more flux than it is now. They could now be
considered to be prettier versions of function pointers.
But a *pure* virtual function is more dubious as you will find it
introduces a hidden dependency on the __cxa_pure_virtual() function in
libsupc++. What happens here might be dependent on how someone has built
their toolchain, and in the case of the gcc we distribute with eCos right
now, it does effectively:
{
fputs("pure virtual method called\n", stderr);
std::terminate()
}
So we now have dependencies on fputs, stderr, and more stuff from
libsupc++ in the form of std::terminate, which involves termination
handlers, and then a call to abort().
So in other words, this is a large chain of unwanted dependencies.
Therefore I think we have a choice of solutions: we can either change the
pure virtual functions to be something lik e.g.:
virtual Cyg_ErrNo write( const cyg_uint8 *buffer, cyg_ucount32
buffer_length, cyg_ucount32 *bytes_written ) {
CYG_FAIL("write method called directly on Cyg_OutputStream");
}
However even that depends on assertion support being on, so is not ideal.
At the same time, it adds virtual functions in each stream. It does take
away the DEVIO_TABLE global, but the virtual functions are for _every_
stream, which in many cases will mean 36 bytes for functionality that may
never be used (and since we're trying to fit eCos on some SoC with 16K
RAM, we care!).
Code usage is about the same fortunately.
But therefore I think it would be better all round if these changes did
use the above CYG_FAIL, but were also wrapped in a configuration option
(which can be default on admittedly), and if you could adjust the patch
accordingly that would be great.
You may find it easier to clone vsnprintf.cxx into a different file, only
compiled with this option on, rather than putting a string of ifdefs in
there. It would also make it easier if a Cyg_OutputStream is a straight
typedef of Cyg_StdioStream when that option is disabled, so that
vfnprintf.cxx can still use a Cyg_OutputStream in all cases.
Secondly, I think Cyg_VsnprintfStream::write() would be improved by using
memcpy() - the compiler can do clever things when it sees a memcpy() call.
You might find it speeds things up further on ARM7 - it certainly could on
other architectures.
If you can think of any even better way to improve what I suggest, that
would be good too!
I cannot find a record of a copyright assignment. Did you fill one in? I
know you have made several contributions in the past, and I thought your
one to the kernel would have been large enough to warrant one. Well,
unfortunately, this change is large enough to need one, so if you could
look at http://ecos.sourceware.org/assign.html that would be great,
thanks! Fortunately, once done, you should not ever need to do another
one. Sorry about the bureaucracy - this gives some of the rationale:
http://ecos.sourceware.org/patches-assignment.html
Jifl
> ------------------------------------------------------------------------
>
> Index: packages/language/c/libc/stdio/current/ChangeLog
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/ChangeLog,v
> retrieving revision 1.39
> diff -u -r1.39 ChangeLog
> --- packages/language/c/libc/stdio/current/ChangeLog 22 Dec 2006 21:37:00 -0000 1.39
> +++ packages/language/c/libc/stdio/current/ChangeLog 25 Dec 2006 18:52:07 -0000
> @@ -1,3 +1,22 @@
> +2006-12-25 Sergei Organov <osv@javad.com>
> +
> + Speed-up [v]s[n]printf() functions by a factor of about 2+. In
> + particular, sprintf(s, "%s", "") becomes faster 2.8 times,
> + printing of every character -- 1.7 times, and, as a result, e.g.,
> + printing of a string of length 50 -- 2.2 times.
> +
> + * include/stream.hxx (class Cyg_OutputStream): New ABC.
> + (class Cyg_StdioStream): inherit from Cyg_OutputStream; make
> + the destructor, write(), and get_error() virtual.
> +
> + * src/output/vfnprintf.cxx (vfnprintf): Use ABC Cyg_OutputStream
> + instead of Cyg_StdioStream.
> +
> + * src/common/vsnprintf.cxx (class Cyg_VsnprintfStream): New class
> + that specializes Cyg_OutputStream for output to a string.
> + (vsnprintf): Use Cyg_VsnprintfStream for printing to a string.
> +
> +
> 2006-12-22 Sergei Organov <osv@javad.com>
>
> * src/output/vfnprintf.cxx (vfnprintf): Speed-up formatting of
> Index: packages/language/c/libc/stdio/current/include/stream.hxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/include/stream.hxx,v
> retrieving revision 1.7
> diff -u -r1.7 stream.hxx
> --- packages/language/c/libc/stdio/current/include/stream.hxx 29 Mar 2004 11:24:38 -0000 1.7
> +++ packages/language/c/libc/stdio/current/include/stream.hxx 25 Dec 2006 18:52:07 -0000
> @@ -73,11 +73,26 @@
>
> // TYPE DEFINITIONS
>
> +class Cyg_OutputStream
> +{
> +public:
> +
> + Cyg_OutputStream() {}
> +
> + virtual ~Cyg_OutputStream() {}
> +
> + virtual Cyg_ErrNo write( const cyg_uint8 *buffer, cyg_ucount32 buffer_length,
> + cyg_ucount32 *bytes_written ) = 0;
> +
> + virtual Cyg_ErrNo get_error( void ) = 0;
> +
> +};
> +
> class Cyg_StdioStream;
> __externC Cyg_ErrNo
> cyg_libc_stdio_flush_all_but( Cyg_StdioStream * );
>
> -class Cyg_StdioStream
> +class Cyg_StdioStream: public Cyg_OutputStream
> {
> friend int setvbuf( FILE *, char *, int, size_t ) __THROW;
> friend Cyg_ErrNo
> @@ -207,7 +222,7 @@
> public:
>
> // DESTRUCTOR
> -
> + virtual
> ~Cyg_StdioStream();
>
>
> @@ -253,7 +268,7 @@
> cyg_ucount32
> bytes_available_to_read( void );
>
> - Cyg_ErrNo
> + virtual Cyg_ErrNo
> write( const cyg_uint8 *buffer, cyg_ucount32 buffer_length,
> cyg_ucount32 *bytes_written );
>
> @@ -284,7 +299,7 @@
> unlock_me( void );
>
> // get error status for this file
> - Cyg_ErrNo
> + virtual Cyg_ErrNo
> get_error( void );
>
> // set error status for this file.
> Index: packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
> retrieving revision 1.4
> diff -u -r1.4 vsnprintf.cxx
> --- packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx 15 Mar 2004 15:21:44 -0000 1.4
> +++ packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx 25 Dec 2006 18:52:07 -0000
> @@ -43,9 +43,9 @@
> // Author(s): jlarmour
> // Contributors: jlarmour
> // Date: 1998-02-13
> -// Purpose:
> -// Description:
> -// Usage:
> +// Purpose:
> +// Description:
> +// Usage:
> //
> //####DESCRIPTIONEND####
> //
> @@ -61,92 +61,49 @@
> #include <stddef.h> // NULL and size_t from compiler
> #include <stdio.h> // header for this file
> #include <errno.h> // error codes
> -#include <cyg/io/devtab.h> // Device table
> #include <cyg/libc/stdio/stream.hxx>// Cyg_StdioStream
>
> #include <cyg/libc/stdio/io.inl> // I/O system inlines
>
> -#ifndef CYGPKG_LIBC_STDIO_FILEIO
> -
> // FUNCTIONS
>
> -static Cyg_ErrNo
> -str_write(cyg_stdio_handle_t handle, const void *buf, cyg_uint32 *len)
> +class Cyg_VsnprintfStream: public Cyg_OutputStream
> {
> - cyg_devtab_entry_t *dev = (cyg_devtab_entry_t *)handle;
> - cyg_uint8 **str_p = (cyg_uint8 **)dev->priv;
> - cyg_ucount32 i;
> -
> - // I suspect most strings passed to vsnprintf will be relatively short,
> - // so we just take the simple approach rather than have the overhead
> - // of calling memcpy etc.
> -
> - // simply copy string until we run out of user space
> -
> - for (i = 0; i < *len; i++, (*str_p)++ )
> - {
> - **str_p = *((cyg_uint8 *)buf + i);
> - } // for
> -
> - *len = i;
> -
> - return ENOERR;
> -
> -} // str_write()
> +public:
> + Cyg_VsnprintfStream(char* s): s_(s) {}
>
> -static DEVIO_TABLE(devio_table,
> - str_write, // write
> - NULL, // read
> - NULL, // select
> - NULL, // get_config
> - NULL); // set_config
> + virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
>
> -externC int
> -vsnprintf( char *s, size_t size, const char *format, va_list arg ) __THROW
> -{
> - int rc;
> - // construct a fake device with the address of the string we've
> - // been passed as its private data. This way we can use the data
> - // directly
> - DEVTAB_ENTRY_NO_INIT(strdev,
> - "strdev", // Name
> - NULL, // Dependent name (layered device)
> - &devio_table, // I/O function table
> - NULL, // Init
> - NULL, // Lookup
> - &s); // private
> - Cyg_StdioStream my_stream( &strdev, Cyg_StdioStream::CYG_STREAM_WRITE,
> - false, false, _IONBF, 0, NULL );
> -
> - rc = vfnprintf( (FILE *)&my_stream, size, format, arg );
> -
> - // Null-terminate it, but note that s has been changed by str_write(), so
> - // that it now points to the end of the string
> - s[0] = '\0';
> + virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
> + cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );
>
> - return rc;
> + virtual Cyg_ErrNo get_error( void ) { return ENOERR; }
>
> -} // vsnprintf()
> +private:
> + char* s_;
> +};
>
> -#else
> +Cyg_ErrNo
> +Cyg_VsnprintfStream::write(
> + const cyg_uint8 *buffer,
> + cyg_ucount32 buffer_length,
> + cyg_ucount32 *bytes_written )
> +{
> + char *dest = s_;
> + char const *src = (char const *)buffer;
> + char const *end = src + buffer_length;
> + while(src < end)
> + *dest++ = *src++;
> + s_ = dest;
> + *bytes_written = buffer_length;
> + return ENOERR;
> +}
>
> externC int
> vsnprintf( char *s, size_t size, const char *format, va_list arg ) __THROW
> {
> - int rc;
> -
> - Cyg_StdioStream my_stream( Cyg_StdioStream::CYG_STREAM_WRITE,
> - size, (cyg_uint8 *)s );
> -
> - rc = vfnprintf( (FILE *)&my_stream, size, format, arg );
> -
> - if( rc > 0 )
> - s[rc] = '\0';
> -
> - return rc;
> -
> + Cyg_VsnprintfStream stream(s);
> + return vfnprintf( (FILE *)&stream, size, format, arg );
> } // vsnprintf()
>
> -#endif
> -
> // EOF vsnprintf.cxx
> Index: packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx,v
> retrieving revision 1.9
> diff -u -r1.9 vfnprintf.cxx
> --- packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx 22 Dec 2006 21:37:00 -0000 1.9
> +++ packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx 25 Dec 2006 18:52:08 -0000
> @@ -210,7 +210,7 @@
> #define PRINT(ptr, len) \
> CYG_MACRO_START \
> cyg_ucount32 length = MIN( (cyg_ucount32) len, n - ret - 1); \
> - if (((Cyg_StdioStream *)stream)->write( (const cyg_uint8 *)ptr, \
> + if (((Cyg_OutputStream *)stream)->write( (const cyg_uint8 *)ptr, \
> length, &length )) \
> goto error; \
> if (length < (cyg_ucount32)len) { \
> @@ -671,7 +671,7 @@
> }
> done:
> error:
> - return (((Cyg_StdioStream *) stream)->get_error() ? EOF : ret);
> + return (((Cyg_OutputStream *) stream)->get_error() ? EOF : ret);
> /* NOTREACHED */
> }
>
--
eCosCentric http://www.eCosCentric.com/ The eCos and RedBoot experts
Company legal info, address and number: http://www.ecoscentric.com/legal
------["The best things in life aren't things."]------ Opinions==mine
More information about the Ecos-patches
mailing list