Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support C++20 std::format as an alternative to fmtlib #2170

Merged
merged 23 commits into from
Nov 16, 2021
Merged

Support C++20 std::format as an alternative to fmtlib #2170

merged 23 commits into from
Nov 16, 2021

Conversation

sylveon
Copy link
Contributor

@sylveon sylveon commented Nov 13, 2021

This, when using C++20 mode, allows spdlog to use std::format instead of fmtlib, making spdlog effectively standalone. Compile-time format string checking is the only missing feature compared to using fmtlib, because in std::format, std::format_string was made exposition-only so user code can't access/use it.

@gabime
Copy link
Owner

gabime commented Nov 13, 2021

Thanks.See my comments:

  1. Some of the tests fail under windows.

  2. Replacing fmt::basic_memory_buffer with std::string is problematic since allocations would happen in each log call. fmt::basic_memory_buffer was chosen to use 250 bytes of stack and grow on heap only if really needed.

  3. Multiple ifdefs - Maybe we can find cleaner way, like hiding all the ifdefs inside the “spdlog/fmt/fmt.h” common header file.

  4. I am not even sure all those changes worth it in present time considering there is still little support for std::format by compilers (see https://en.cppreference.com/w/cpp/compiler_support).

@sylveon
Copy link
Contributor Author

sylveon commented Nov 13, 2021

Some of the tests fail under windows.

I had to wait for AppVeyor to catch up to all my commits to see what was fixed by the follow ups, will look at them soon.

Replacing fmt::basic_memory_buffer with std::string is problematic since allocations would happen in each log call. fmt::basic_memory_buffer was chosen to use 250 bytes of stack and grow on heap only if really needed.

On MSVC, std::format will behave much faster with std::string than with memory_buffer (or anything else) especially with longer strings because it implements special optimizations for it (comparing against fmtlib 8.0.1):

Number of iterations: 1 000 000

Average time (ns):
-----------------------------------
std::format duration: 1241
fmt::format duration: 1240
  Ratio (std/fmt): 1.00

Format_to with std::back_inserter(std::string):
std::format_to duration: 1173
fmt::format_to duration: 5861
  Ratio (std/fmt): 0.20

Format_to with std::back_inserter(fmt::memory_buffer):
std::format_to duration: 1738
fmt::format_to duration: 2272
  Ratio (std/fmt): 0.76

-----------------------------------

https://gist.github.com/sylveon/6e6347e2975d7fba0ebad039f233f52a

Generally, allocation time is trivially short compared to the operation of formatting strings itself.

Multiple ifdefs - Maybe we can find cleaner way, like hiding all the ifdefs inside the “spdlog/fmt/fmt.h” common header file.

I already tried reducing those as most as possible. What's remaining is mostly in test code. Is it possible to just overload operator== in test code to resolve this?

I am not even sure all those changes worth it in present time considering there is still little support for std::format by compilers (see en.cppreference.com/w/cpp/compiler_support).

MSVC's is complete, and local testing shows everything seems to work fine with it. Clang seems to be on its way to supporting as well. And I imagine GCC is probably not that far out either. I wrote this assuming P2216 so it should be compatible with all implementations once they are out.

@gabime
Copy link
Owner

gabime commented Nov 13, 2021

because it implements special optimizations for it

Interesting. what kind of optimizations?

Seems that the back inserter benchmark use he same backing string object for all the iterations so no wonder it is so fast. Allocation happened only once (still, I dont understand why it is x5 faster than fmt)

@sylveon
Copy link
Contributor Author

sylveon commented Nov 13, 2021

Interesting. what kind of optimizations?

It recognizes that you passed back_inserter<std::string> and will call append in blocks of 256 characters instead of doing a push_back character per character.

the same backing string object for all the iterations so no wonder it is so fast.

The memory_buffer benchmark does the same, and memory_buffer::clear doesn't get rid of the backing allocation if any, just sets the size back to 0.

@gabime
Copy link
Owner

gabime commented Nov 14, 2021

The memory_buffer benchmark does the same..

Right but the format string under test is bigger than 250 so they both allocate. I suggest benching with 100-200 chars and of course not reuse the backing string object across the iterations.

@sylveon
Copy link
Contributor Author

sylveon commented Nov 14, 2021

Using a 94 characters format string, and creating a string/memory_buffer in the lambda instead of globally gives this:

Number of iterations: 1 000 000

Average time (ns):
-----------------------------------
std::format duration: 349
fmt::format duration: 259
  Ratio (std/fmt): 1.35

Format_to with std::back_inserter(std::string):
std::format_to duration: 337
fmt::format_to duration: 1662
  Ratio (std/fmt): 0.20

Format_to with std::back_inserter(fmt::memory_buffer):
std::format_to duration: 375
fmt::format_to duration: 220
  Ratio (std/fmt): 1.70

-----------------------------------

The same benchmark using wide characters:

Number of iterations: 1 000 000

Average time (ns):
-----------------------------------
std::format duration: 394
fmt::format duration: 468
  Ratio (std/fmt): 0.84

Format_to with std::back_inserter(std::string):
std::format_to duration: 421
fmt::format_to duration: 3258
  Ratio (std/fmt): 0.13

Format_to with std::back_inserter(fmt::memory_buffer):
std::format_to duration: 440
fmt::format_to duration: 646
  Ratio (std/fmt): 0.68

-----------------------------------

std::format is a tiny bit faster on strings than on memory_buffer, however I'm not sure why fmtlib suffers so much on the string here (but it happens reliably accross runs). These where built with /O2 (max optimization on MSVC).

fmtlib + fmt::memory_buffer is obviously best case for short strings, but ultimately this 150ns difference won't be what makes or break spdlog. It's still plenty fast (and with wchar_t, actually faster).

@gabime
Copy link
Owner

gabime commented Nov 14, 2021

So to summarize for MSVC, using std::format_to with std::back_inserter(std::string) is the fastest. For other platforms, when available we will have to bench and see.

I wonder if we could optimize the heap allocation of the std::string though. Maybe reuse/extract fmt's memory_buffer, or have some kind of stack based allocator for std::string.

@sylveon
Copy link
Contributor Author

sylveon commented Nov 14, 2021

Yeah, a custom allocator could work, it seems. Could be a bit tricky when a string is moved, but usage of memory_buf_t avoids this already.

@gabime
Copy link
Owner

gabime commented Nov 15, 2021

Probably would be easier to reuse/extract fmt's memory_buffer. Could be done on later PR though. Once you fix the tests I can merge, so let me know @sylveon

@sylveon
Copy link
Contributor Author

sylveon commented Nov 15, 2021

Sure thing. Extracting the memory_buffer from fmt won't make it anymore faster I believe however.

@sylveon
Copy link
Contributor Author

sylveon commented Nov 15, 2021

Don't mind the mess of commits, just fixing things and letting the CI validate it. Feel free to squash all of this when merging.

include/spdlog/details/os-inl.h Outdated Show resolved Hide resolved
include/spdlog/details/os-inl.h Outdated Show resolved Hide resolved
include/spdlog/logger.h Show resolved Hide resolved
include/spdlog/sinks/daily_file_sink.h Outdated Show resolved Hide resolved
include/spdlog/details/fmt_helper.h Outdated Show resolved Hide resolved
@sylveon
Copy link
Contributor Author

sylveon commented Nov 16, 2021

Seems like the Travis CI build doesn't work anymore: https://app.travis-ci.com/github/gabime/spdlog/requests

@gabime
Copy link
Owner

gabime commented Nov 16, 2021

fixed the travis ci..

@sylveon
Copy link
Contributor Author

sylveon commented Nov 16, 2021

Changes requested have been done @gabime

The Windows builds are succeeding, but the Travis CI builds are failing, seemingly because of changes recently introduced in v1.x unrelated to this PR (which weren't checked due to the issue above)

@gabime gabime merged commit ff80d10 into gabime:v1.x Nov 16, 2021
@gabime
Copy link
Owner

gabime commented Nov 16, 2021

Merged. Thanks @sylveon !

@vitaut
Copy link

vitaut commented Nov 17, 2021

The format_to numbers with std::string seem suspiciously low. For example, on clang/libc++ I get the following:

% c++ bench.cc -I include -O3 -DNDEBUG -std=c++17 src/format.cc
% ./a.out
Number of iterations: 1,000,000

Average time (ns):
-----------------------------------
fmt::format duration: 212

Format_to with std::back_inserter(std::string):
fmt::format_to duration: 209

Format_to with std::back_inserter(fmt::memory_buffer):
fmt::format_to duration: 146

with format_to and format being essentially the same (as expected) and memory_buffer faster (also expected).

@sylveon, what version of {fmt} are you using and what are the full compiler options? It seems that at least NDEBUG is missing.

@sylveon
Copy link
Contributor Author

sylveon commented Nov 17, 2021

I am using fmt 8.0.1 with MSVC 19.30.30705 (VS 17.1 preview 1). The full compiler options are:

/permissive- /ifcOutput "x64\Release\" /GS /GL /W3 /Gy /Zc:wchar_t /Zi /Gm- /O2 /sdl /Fd"x64\Release\vc143.pdb" /Zc:inline /fp:precise /D "NDEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /errorReport:prompt /WX- /Zc:forScope /Gd /Oi /MD /std:c++latest /FC /Fa"x64\Release\" /EHsc /nologo /Fo"x64\Release\" /Fp"x64\Release\ConsoleApplication62.pch" /diagnostics:column 

Both NDEBUG and /O2 are there.

@vitaut
Copy link

vitaut commented Nov 17, 2021

Would be interesting to investigate why perf on msvc is so bad compared to clang's. Could be some easy optimization opportunities.

@sylveon
Copy link
Contributor Author

sylveon commented Jul 23, 2022

P2508R1, if merged, will expose std::basic-format-string as a concrete type, allowing spdlog to also support compile-time verification of format strings with std::format, bringing it up to par with fmtlib

cplusplus/papers#1163 microsoft/STL#2937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants