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

True. Another way of putting it is that they (and other C++ features, e.g. templates) do invite it. It's easy to fall in the trap of making lots of virtuals and not thinking about memory use.


But that's not the only issue I was thinking of. One I was thinking of is that by adding a virtual function you now ensure that _all_ the virtual functions for every class need to be included, whether or not they are used (and all their dependencies, etc.etc.).

With non-virtual C++ calls, as it was before, if someone does not call the write() method, it is removed from the program image. Making it virtual prevents that.

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]

Firstly, since this would be a fundamental problem, if it occurs with assertions off, it could be unnoticed. Secondly (and this applies even with assertions off), I couldn't be sure that the compiler will always remove this base class virtual function from the image, even if it is unused in practice. That could be verified one way or the other admittedly. Nevertheless....


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.

My miscalculation made it sound better than it was in fact. In the common case: three streams for stdin, stdout, stderr, give 12 bytes, add 28*3 to give 96 bytes.


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.

eCos has now (for better or worse) been around quite a long time, and it may be that that comment precedes the compiler's ability to optimise memcpy() with a builtin. Either way, it is now wrong - calling memcpy() is a good thing.


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

Oh yes, I know there's a lot of things I would do very differently given the chance and the benefit of 10 more years of experience. There are many, many places the code could be improved (or rewritten!), and that is one.


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

"virtual" is not mere syntax! :-)


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.

Due to the increased RAM use from vtables, and code from the functions becoming virtual and thereby always being included even when the code is not used, I think this has to be an option.


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.

Hmm, that kernel change did seem big enough to need one in my view. I think someone may have dropped the ball there. We would want an assignment for that, irrespective of this libc stdio change.


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.

The first line of the second web link is:
"Any patch greater than a few lines (around 10 as a general rule) needs to be accompanied by an assignment of copyright"


The get-out clause we sometimes use is that small things can fit under the bar of what could be considered a copyrightable work. Even small things repeated identically 1,000 times could be ok. But any identifiable "work" (in the Intellectual Property sense) would theoretically allow you to have (C) Sergei Organov at the top. But the mandatory policy is that all such changes must be assigned - the FSF will not have it any other way.

Jifl
--
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]