This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos 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] Speed-up sprintf() family of functions.


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


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