Re: [PATCH] Speed-up sprintf() family of functions.

[Jonathan Larmour <jifl at eCosCentric dot com>]

Sergei Organov wrote:
> Jonathan Larmour <jifl@eCosCentric.com> writes:
> > Sergei Organov wrote:
> > > Jonathan Larmour <jifl@eCosCentric.com> writes:
>
> [...]
>
> > > > 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.
>
> The question is how is it different from any other occurrence of CYG_FAIL
> in the eCos source base?

Well, I guess you have a point. Okay, go with the CYG_FAIL.

> > 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....
>
> I think compiler won't remove the base class virtual function, unless it
> can remove entire virtual table. So it does have an overhead of having
> these 3 empty functions. What I don't get is why is it a problem for an
> application that uses either Cyg_StdioStream or vfnprintf() that both
> are rather fat anyway.

Overhead is incurred irrespective of whether vfnprintf is used. 
Cyg_StdioStream is not fat if you care to tune configuration options such 
as buffering support, ungetc, etc.

> > > > 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.
>
> [...]
>
> > 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.
>
> RAM use from vtables, as calculated above, is 12 bytes, as vtables
> themselves are ROMable.

Aaah, yes, of course. You're right. I forgot they are constant data <smacks 
head>.

> That said, I'm still not convinced it needs a configuration option. In
> general, there is size vs speed issue with almost any non-trivial code,
> and we obviously aren't going to have at least two variants for every
> such case and to put all of them under options?

For 12 bytes in the typical case, I'm okay with that.

The only potential problem could be extra footprint caused by pulling in 
the various versions of the write() and get_error() methods, along with the 
desstructor. But in this *particular* case, this isn't so bad as those 
functions do not themselves impose extra requirements of any note, and 
people are always going to want to write() a stream. So it's not a 
free-for-all for virtual functions by any means - the same logic would not 
apply to all methods, but this particular case is ok.

If you could repost the patch with the changes we did agree on, that would 
be great, thanks. Although I won't be able to commit it before your 
copyright assignment arrives.

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