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.


Jonathan Larmour <jifl@eCosCentric.com> writes:
> 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.

AFAIR, virtual functions, unlike templates, never were a problem in
practice, except that sometimes programmers didn't understand how to
apply them right, but that was (and unfortunately still is) true for
almost every C++ feature.

> 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().

This is dependency I didn't notice myself due to the fact that on the
target where I really care about every byte of memory, I have empty
pure_virtual() that replaces the standard one ;)

>
> 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.

Why not ideal? Either you want assertions and then you compile with
assertions on, or you don't. I think it's the best way to handle this
provided you don't want to bring C++ runtime to every application. [I
have my applications in C++, so I do battle with these issues anyway]

> 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!).

How come 36 bytes? 4 bytes x 9 streams? Do you really have 9 streams on
SoC with 16K RAM?!

On ARM I get 4 bytes * number of streams: pointer to virtual table for
every stream, plus 28 bytes * 3 for total 3 virtual tables, but the
latter doesn't depend on the number of streams.

> 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.

I still don't see a reason to wrap it in a configuration option.

> 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.

I don't think so. Please read a comment in the original vsnprintf.cxx
file:

    // 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

and the implementation of memcpy() there. Also please notice that the
old implementation was really ugly, preventing almost all loop
optimizations due to possible aliasing between pointers.

In fact, that was the only *code* that I've changed in the patch, the
rest of the patch is just declarations.

>
> If you can think of any even better way to improve what I suggest,
> that would be good too!

Currently I think that simple CYG_FAIL is optimal.

>
> 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.

No, I didn't fill any.

> 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

I have no serious objections to file an assignment, even though I fail
to understand your criteria of "large". Anyway, I think we can return to
this issue once we agree on the technical side of the patch.

-- Sergei.


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