compile with {fmt} v8 #4260

tchaikov · 2022-04-12T11:13:43Z

Cover letter

now that {fmt} v8 is released. and would be great if we can compile against it. in this changeset

since fmt v8 differentiate compile-time fmt string from the runt-time fmt string. the former is a static one and can be validated at compile-time, the later can only be validated at runt-time. so in this change, the format string are passed in different ways accordingly for better performance and compile-time validation.
to simplify the handling of difference API of v8 and pre-v8, fmt is also packaged as yet another dependency
fmt v8 does not format scoped enums. so add a bunch of fmt::format() specialization for better debugging experience.

Fixes #ISSUE-NUMBER, Fixes #ISSUE-NUMBER, ...

Release notes

none

src/v/ssx/sformat.h

dotnwat · 2022-04-12T15:00:40Z

👋 Kefu! 👋

dotnwat · 2022-04-12T18:26:00Z

@tchaikov there is a .clang-format in the root of the tree. looks like the files you touched may need run through clang format (CI is complaining here: https://github.com/redpanda-data/redpanda/runs/5990346426?check_suite_focus=true)

BenPope · 2022-04-12T19:53:27Z

I took a little look at updating libfmt a while ago, but didn't have the bandwidth to figure out what the internal changes are.

I think we should wait until we branch v21.1.x to merge (that doesn't need to block changes on this PR).

libfmt-devel >=8 isn't in Ubuntu until Jammy (April 21st) and Fedora 35, so we may also need to package it in the oss build.

tchaikov · 2022-04-15T15:22:45Z

@tchaikov there is a .clang-format in the root of the tree. looks like the files you touched may need run through clang format (CI is complaining here: https://github.com/redpanda-data/redpanda/runs/5990346426?check_suite_focus=true)

thanks for the pointer. sure, will do.

emaxerrno · 2022-04-15T21:03:20Z

good to see you here @tchaikov 👋🏽 🚀

tchaikov · 2022-04-16T13:22:11Z

libfmt-devel >=8 isn't in Ubuntu until Jammy (April 21st) and Fedora 35, so we may also need to package it in the oss build.

okay, will add a commit in this PR to package the latest stable version of fmt in oss.cmake.in . much simpler this way.

tchaikov · 2022-04-17T10:27:31Z

@BenPope hi Ben, thanks for your review. i just sent all changes related to fmt v8. now the tree compiles just fine with them and some other misc changes (also proposed as PRs). but i still have two questions. could you please shed some lights on them?

shall i drop the backward compatible implementation for fmt_append()
shall i keep fmt_append() in src/v/ssx/sformat.h? if not, would you please suggest where i shall put it?

jcsp · 2022-05-03T12:50:21Z

Ping @BenPope I think this is waiting for your input?

BenPope · 2022-05-03T13:22:14Z

Thanks @jcsp, I'm aware.

BenPope · 2022-05-13T05:27:59Z

@tchaikov - thanks for this. I hacked on this a bit - my branch is it: https://github.com/BenPope/redpanda/commits/update-fmt-v8

It could do with some tidy up of the history. I spent quite a while on a big performance regression.

tchaikov · 2022-05-13T12:10:08Z

@BenPope much appreciated! i will fold your changes into my branch with your credits.

tchaikov · 2022-05-13T12:52:45Z

hi Ben, in the latest revision, i

incorporated all your fmt-v8 related change in https://github.com/BenPope/redpanda/commits/update-fmt-v8
dropped my change introducing fmt_append() in favor of your sformat change.
folded your ["differentiate compile-time fmt str from runtime str" ](BenPope@5b0d1d7) into the "differentiate compile-time fmt str..." change
added [squash-me] v: differentiate compile-time fmt str from runtime str to address a new FTBFS
added schema_registry/proto: move {io,dp}_error_collector::error() down to address yet another FTBFS

i will squash the related change into a single one after the review.

BenPope · 2022-05-14T19:33:13Z

I'm seeing a compilation failure, I guess we should wait for #4722

In file included from ../../../src/v/pandaproxy/rest/handlers.cc:21:
  | In file included from ../../../src/v/pandaproxy/json/exceptions.h:15:
  | ../../../src/v/pandaproxy/error.h:73:8: error: explicit specialization of 'std::is_error_code_enum<kafka::error_code>' after instantiation
  | struct is_error_code_enum<kafka::error_code> : true_type {};
  | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  | /vectorized/llvm/bin/../include/c++/v1/system_error:328:39: note: implicit instantiation first required here
  | typename enable_if<is_error_code_enum<_Ep>::value>::type* = nullptr

tchaikov · 2022-05-15T01:57:33Z

ahh, this failure is one of the day0 failures i wanted to address in #4307 which is now superseded by #4722.

BenPope · 2022-05-16T22:10:38Z

@tchaikov can you rebase this on dev, #4722 has been merged.

tchaikov · 2022-05-17T01:53:27Z

@BenPope sure. squashed, rebased and repushed.

dotnwat · 2022-05-17T05:05:32Z

retriggered internal ci

tchaikov · 2022-05-17T05:59:04Z

change since previous revision

clang-fmt'ed

BenPope

LGTM.

Out internal build and tests have passed.

This will have to be squashed, as it's not worth trying to make all intermediate steps compatible with both v7 and v8, and it will make git bisect easier.

BenPope · 2022-05-17T10:45:04Z

src/v/raft/consensus.cc

+    constexpr auto parse(format_parse_context& ctx) { return ctx.end(); }
+    template<typename FormatContext>
+    auto format(const vote_state& s, FormatContext& ctx) const {
+        const char* str = "unknown";


I don't know if strlen would be called inside fmt, but it might make sense to use a std::string_view.

If that seems suitable, then it may be worth inheriting from formatter<std::string_view> as is done for seastar::sstring: struct fmt::formatter<seastar::sstring> final : fmt::formatter<string_view>, to avoid implementing parse. and to get the formatting options it provides.

src/v/rpc/demo/demo_utils.h

src/v/storage/segment_utils.cc

src/v/model/metadata.h

tchaikov · 2022-05-17T14:55:31Z

change since the previous version

squashed into a single commit. the commit messages are itemized and merged.
addressed the string_view comments
inherit from fmt::formatter<string_view>
dropped the clang-format off change, as the tree compiles fine without it.

as fmt v8 started to error if we pass non-constexpr fmt str to fmt::format() and friends * rpc: include ssx/sformat.h early to get the fmt::formatter<seastar::sstring> instantiated before it is implicit instantiated by Seastar library. * storage: specialize fmt::formtter() for recovert_state * r/consensus: specialize fmt::format() for consensus::vote_state * cluster: specialize fmt::format() for feature_state::state * k/fetch: specialize fmt::format() for isolation * cmake: add fmt to oss.cmake to be compatible with both v8 and pre-v8 libfmt is not necessary if we could just use the former. and fmt v8 features compile-time fmt_string check, which could help us avoid mistakes like: fmt::format("you have two choices: {} and {}", one_option); * util: Avoid deprecated fmt::format_to overload * ssx/sformat: Update to fmtlib v8 * Inherit formatter from string_view to get formatting * Hack to prefer formatter over operator<< * This is a big performance win * Reimplement sformat for performance * With v7, it was about 2x fmt::format with std::string * It's now on par * Using formatted_size was slower than this implementation * schema_registry/proto: move {io,dp}_error_collector::error() down Signed-off-by: Ben Pope <ben@redpanda.com> Signed-off-by: Kefu Chai <tchaikov@gmail.com>

BenPope

https://buildkite.com/redpanda/vtools/builds/2255

BenPope

LGTM - thanks @tchaikov !

tchaikov · 2022-05-18T10:39:35Z

shall i worry about the test failures? after registering on buildkite, i still have 404 when trying to read any of them.

BenPope · 2022-05-18T11:02:11Z

shall i worry about the test failures? after registering on buildkite, i still have 404 when trying to read any of them.

Sorry, our builds are private, that was intended for other redpanda-data members.

tchaikov requested review from BenPope and dotnwat as code owners April 12, 2022 11:13

github-actions bot added the area/redpanda label Apr 12, 2022

tchaikov commented Apr 12, 2022

View reviewed changes

src/v/ssx/sformat.h Outdated Show resolved Hide resolved

tchaikov force-pushed the fmt-v8 branch from 188b601 to 53e2e5a Compare April 16, 2022 07:59

tchaikov force-pushed the fmt-v8 branch from 53e2e5a to f4bee1c Compare April 17, 2022 10:08

tchaikov requested review from rystsov, mmaslankaprv, LenaAn, VadimPlh, jcsp, ztlpn, NyaliaLui and ivotron as code owners April 17, 2022 10:08

github-actions bot added the area/build label Apr 17, 2022

tchaikov mentioned this pull request Apr 17, 2022

rpc: include ssx/sformat.h early #4305

Closed

tchaikov force-pushed the fmt-v8 branch 2 times, most recently from d945bf6 to 6df86e4 Compare April 17, 2022 10:47

tchaikov force-pushed the fmt-v8 branch from 6df86e4 to 2352727 Compare May 3, 2022 13:36

tchaikov requested a review from a team as a code owner May 3, 2022 13:36

BenPope mentioned this pull request May 12, 2022

add libfmt to oss.cmake.in #4716

Closed

tchaikov force-pushed the fmt-v8 branch from 2352727 to 73d3204 Compare May 13, 2022 00:13

BenPope mentioned this pull request May 13, 2022

pandaproxy: Invert error_code dependencies #4722

Merged

tchaikov force-pushed the fmt-v8 branch from 73d3204 to 8b7dde5 Compare May 13, 2022 12:38

tchaikov force-pushed the fmt-v8 branch from 69d89ef to 6664a7f Compare May 17, 2022 01:52

tchaikov force-pushed the fmt-v8 branch from 6664a7f to 5993d20 Compare May 17, 2022 05:58

BenPope reviewed May 17, 2022

View reviewed changes

tchaikov force-pushed the fmt-v8 branch 2 times, most recently from 01c65fe to f532b14 Compare May 17, 2022 14:53

tchaikov force-pushed the fmt-v8 branch from f532b14 to f4573f8 Compare May 17, 2022 16:24

tchaikov force-pushed the fmt-v8 branch from f4573f8 to 8c05afc Compare May 17, 2022 16:45

BenPope reviewed May 17, 2022

View reviewed changes

BenPope approved these changes May 17, 2022

View reviewed changes

dotnwat approved these changes May 23, 2022

View reviewed changes

BenPope merged commit 2345345 into redpanda-data:dev May 23, 2022

tchaikov mentioned this pull request Jun 15, 2022

Redpanda: use compile time format string with FMT_COMPILe #1808

Closed

tchaikov deleted the fmt-v8 branch July 5, 2022 01:29

mmedenjak added the community label Jul 15, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compile with {fmt} v8 #4260

compile with {fmt} v8 #4260

tchaikov commented Apr 12, 2022 •

edited by BenPope

Loading

dotnwat commented Apr 12, 2022 •

edited

Loading

dotnwat commented Apr 12, 2022 •

edited

Loading

BenPope commented Apr 12, 2022 •

edited

Loading

tchaikov commented Apr 15, 2022

emaxerrno commented Apr 15, 2022

tchaikov commented Apr 16, 2022

tchaikov commented Apr 17, 2022 •

edited

Loading

jcsp commented May 3, 2022

BenPope commented May 3, 2022

BenPope commented May 13, 2022

tchaikov commented May 13, 2022

tchaikov commented May 13, 2022 •

edited

Loading

BenPope commented May 14, 2022 •

edited

Loading

tchaikov commented May 15, 2022 •

edited

Loading

BenPope commented May 16, 2022

tchaikov commented May 17, 2022

dotnwat commented May 17, 2022

tchaikov commented May 17, 2022

BenPope left a comment

BenPope May 17, 2022 •

edited

Loading

tchaikov commented May 17, 2022 •

edited

Loading

BenPope left a comment

BenPope left a comment •

edited

Loading

tchaikov commented May 18, 2022

BenPope commented May 18, 2022

compile with {fmt} v8 #4260

compile with {fmt} v8 #4260

Conversation

tchaikov commented Apr 12, 2022 • edited by BenPope Loading

Cover letter

Release notes

dotnwat commented Apr 12, 2022 • edited Loading

dotnwat commented Apr 12, 2022 • edited Loading

BenPope commented Apr 12, 2022 • edited Loading

tchaikov commented Apr 15, 2022

emaxerrno commented Apr 15, 2022

tchaikov commented Apr 16, 2022

tchaikov commented Apr 17, 2022 • edited Loading

jcsp commented May 3, 2022

BenPope commented May 3, 2022

BenPope commented May 13, 2022

tchaikov commented May 13, 2022

tchaikov commented May 13, 2022 • edited Loading

BenPope commented May 14, 2022 • edited Loading

tchaikov commented May 15, 2022 • edited Loading

BenPope commented May 16, 2022

tchaikov commented May 17, 2022

dotnwat commented May 17, 2022

tchaikov commented May 17, 2022

BenPope left a comment

Choose a reason for hiding this comment

BenPope May 17, 2022 • edited Loading

Choose a reason for hiding this comment

tchaikov commented May 17, 2022 • edited Loading

BenPope left a comment

Choose a reason for hiding this comment

BenPope left a comment • edited Loading

Choose a reason for hiding this comment

tchaikov commented May 18, 2022

BenPope commented May 18, 2022

tchaikov commented Apr 12, 2022 •

edited by BenPope

Loading

dotnwat commented Apr 12, 2022 •

edited

Loading

dotnwat commented Apr 12, 2022 •

edited

Loading

BenPope commented Apr 12, 2022 •

edited

Loading

tchaikov commented Apr 17, 2022 •

edited

Loading

tchaikov commented May 13, 2022 •

edited

Loading

BenPope commented May 14, 2022 •

edited

Loading

tchaikov commented May 15, 2022 •

edited

Loading

BenPope May 17, 2022 •

edited

Loading

tchaikov commented May 17, 2022 •

edited

Loading

BenPope left a comment •

edited

Loading