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

[Laurent Pinchart]

Hi Eric,

On Tue, May 10, 2022 at 01:59:37PM +0100, Eric Curtin via libcamera-devel wrote:
> On Tue, 10 May 2022 at 11:10, Kieran Bingham via libcamera-devel wrote:
> > Quoting Naushir Patuck (2022-05-10 10:49:14)
> > > On Tue, 10 May 2022 at 08:16, Tomi Valkeinen via libcamera-devel 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 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. Converting that code to use fmt::format() and
> > > > naive string append gives:
> > > >
> > > > release with string append      233680
> > > > release lto with string append  186936
> > > >
> > > > Also, if I add another use of fmt::memory_buffer and std::back_inserter
> > > > to another file, I see much less increase in the size:
> > > >
> > > > release lto with two uses of memory_buffer, back_inserter       256736
> > > >
> > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> > >
> > > For what it's worth, I absolutely loathe formatting in std iostream, and
> > > libfmt is a wonderful alternative.
> > > It also is close enough to the C++20 std::format implementation that
> > > eventual porting would be low effort.  So I am all for this change :)

It's going to take a while before we can switch to C++20, but I'm
looking forward to it (and before that to some of the C++17 features
too, it would be nice to use std::optional in the public API).

> I am all in for this change also, personally I would have changed to
> printf for now
> to have one less dependency (also an easy port to C++20 std::format). The
> dependency list is getting large, I noticed a build of mine failing
> recently because
> I didn't have libyaml.

I've posted a patch that falls back to a subproject in that case. Would
you be able to give it a try ?

> But std::format and libfmt are quite fast and anything is better than
> streams so +1.
> 
> > I've never used (yet) {fmt}, but I've only heard good things about
> > performance, and of course it's headed into the standard, so I also
> > think there is some good merit to be found in this development.

-- 
Regards,

Laurent Pinchart