[libcamera-devel] [PATCH] cam: convert to libfmt

[Laurent Pinchart]

Hi Eric,

On Sat, May 21, 2022 at 07:33:03PM +0100, Eric Curtin wrote:
> On Tue, 10 May 2022 at 15:21, Laurent Pinchart wrote:
> > On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> > > On 10/05/2022 16:22, Laurent Pinchart wrote:
> > > > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen wrote:
> > > >> This is just a conversation starter, not for merging. I really like
> > > >> libfmt, and I really hate the C++ stream-based string formatting. libfmt
> > > >> is the first library I add to any new C++ project I make.
> > > >
> > > > You've got it wrong. The first library you should add to any new C++
> > > > project you make is libcamera :-)
> > > >
> > > >> You can find more information about libfmt from:
> > > >>
> > > >> https://github.com/fmtlib/fmt
> > > >> https://fmt.dev/latest/index.html
> > > >>
> > > >> This patch is just a crude conversion with ugly macros to showcase what
> > > >> the formatting code might look like.
> > > >>
> > > >> libfmt pages suggest that libfmt would be faster and smaller (exe size)
> > > >> than iostreams, but for the size it didn't seem to be true in cam's case
> > > >> as the tests below show. However, simple prints did reduce the exe size,
> > > >> but the few more complex ones increased the size.
> > > >>
> > > >> Size tests with gcc 11.2.0-19ubuntu1
> > > >>
> > > >> - Without libfmt
> > > >>
> > > >> debug              3523400
> > > >> debug lto  3269368
> > > >> release            223056
> > > >> release lto        172280
> > > >>
> > > >> - With libfmt
> > > >>
> > > >> debug              4424256
> > > >> debug lto  4143840
> > > >> release            303952
> > > >> release lto        252640
> > > >>
> > > >> Above shows that cam's size clearly increases with libfmt. However, the
> > > >> increase really comes only from one case, the use of fmt::memory_buffer
> > > >> and std::back_inserter.
> > > >
> > > > Is this because libfmt is a header-only library ? What if it is built as
> > > > a shared object, what's the impact on code size in cam ?
> > >
> > > libfmt supports header-only, but it's not header-only here.
> > >
> > > >> Converting that code to use fmt::format() and
> > > >> naive string append gives:
> > > >>
> > > >> release with string append 233680
> > > >> release lto with string append     186936
> > > >
> > > > What's the impact of switching to string concatenation on runtime ?
> > >
> > > What do you mean?
> >
> > I mean what is the impact on CPU usage of using the "naive" string
> > append, compared to std::stringstream or fmt::memory_buffer ?
> >
> > > > Would you consider the option of keeping std::stringstream ?
> > >
> > > You mean formatting mostly with libfmt, but sometimes with streams? Or
> > > using libfmt but appending the formatted string to stringstream?
> >
> > Mostly using fmt::, but using std::stringstream without
> > fmt::memory_buffer when a string needs to be assembled. I'm not saying
> > it's necessarily a good idea, just wondering.
> 
> Something I wrote which you can use to format C++ strings in a printf-style
> 
> ```
>   #pragma once
> 
>   #include <string.h>
>   #include <string>
> 
>   // function that will sprintf to a C++ string starting from std::string::size()
>   // so if you want to completely overwrite a string or start at a specific point
>   // use std::string::clear() or std::string::resize(). str is a std::string.
>   #define STRING_PRINTF(str, ...)                                   \
>     do {                                                            \
>       const int size = snprintf(NULL, 0, __VA_ARGS__);              \
>       const size_t start_of_string = str.size();                    \
>       str.resize(start_of_string + size);                           \
>       snprintf(&str[start_of_string], str.size() + 1, __VA_ARGS__); \
>     } while (0)
> ```
> 
> This way you can format C++ strings in a sprintf-style. It would give
> you many of the benefits of fmt with just a few lines of code
> alternatively. I know we don't only use macros, but it makes sense
> here because when the compiler see's the snprintf, it warns against
> incorrect format strings when you do it this way.

Beside the fact that snprintf() is called twice which may be less
efficient (although maybe still more efficient than libfmt), the trouble
here is that __VA_ARGS__ will be evaluated twice.

	unsigned int index;
	std::string str;

	index = 0;
	STRING_PRINTF(str, "%u", index++);
	/* Now index is equal to 2 */

> Just an option...

-- 
Regards,

Laurent Pinchart