Skip to content

Commit

Permalink
Simplify byte handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@vitaut
vitaut committed Feb 3, 2022
1 parent c7173a3 commit 35c0286
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 14 deletions.
11 changes: 0 additions & 11 deletions include/fmt/core.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#ifndef FMT_CORE_H_
#define FMT_CORE_H_

#include <cstddef> // std::byte
#include <cstdio> // std::FILE
#include <cstring>
#include <iterator>
Expand Down Expand Up @@ -367,12 +366,6 @@ FMT_NORETURN FMT_API void assert_fail(const char* file, int line,
# endif
#endif

#ifdef __cpp_lib_byte
using byte = std::byte;
#else
enum class byte : unsigned char {};
#endif

#if defined(FMT_USE_STRING_VIEW)
template <typename Char> using std_string_view = std::basic_string_view<Char>;
#elif defined(FMT_USE_EXPERIMENTAL_STRING_VIEW)
Expand Down Expand Up @@ -1429,10 +1422,6 @@ template <typename Context> struct arg_mapper {
return map(format_as(val));
}

FMT_CONSTEXPR FMT_INLINE auto map(detail::byte val) -> unsigned {
return map(static_cast<unsigned char>(val));
}

template <typename T, typename U = remove_cvref_t<T>>
struct formattable
: bool_constant<has_const_formatter<U, Context>() ||
Expand Down
11 changes: 8 additions & 3 deletions include/fmt/format.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#define FMT_FORMAT_H_

#include <cmath> // std::signbit
#include <cstddef> // std::byte
#include <cstdint> // uint32_t
#include <cstring> // std::memcpy
#include <limits> // std::numeric_limits
Expand Down Expand Up @@ -159,8 +160,8 @@ FMT_END_NAMESPACE
// __builtin_ctz is broken in Intel Compiler Classic on Windows:
// https://github.com/fmtlib/fmt/issues/2510.
#ifndef __ICL
# if FMT_HAS_BUILTIN(__builtin_ctz) || FMT_GCC_VERSION || \
FMT_ICC_VERSION || FMT_NVCOMPILER_VERSION
# if FMT_HAS_BUILTIN(__builtin_ctz) || FMT_GCC_VERSION || FMT_ICC_VERSION || \
FMT_NVCOMPILER_VERSION
# define FMT_BUILTIN_CTZ(n) __builtin_ctz(n)
# endif
# if FMT_HAS_BUILTIN(__builtin_ctzll) || FMT_GCC_VERSION || \
Expand Down Expand Up @@ -2767,7 +2768,6 @@ FMT_FORMAT_AS(unsigned long, unsigned long long);
FMT_FORMAT_AS(Char*, const Char*);
FMT_FORMAT_AS(std::basic_string<Char>, basic_string_view<Char>);
FMT_FORMAT_AS(std::nullptr_t, const void*);
FMT_FORMAT_AS(detail::byte, unsigned char);
FMT_FORMAT_AS(detail::std_string_view<Char>, basic_string_view<Char>);

template <typename Char>
Expand Down Expand Up @@ -2875,6 +2875,11 @@ constexpr auto underlying(Enum e) noexcept ->
return static_cast<typename std::underlying_type<Enum>::type>(e);
}

#ifdef __cpp_lib_byte
inline auto format_as(std::byte b) -> unsigned char { return underlying(b); }
FMT_FORMAT_AS(std::byte, unsigned char);
#endif

class bytes {
private:
string_view data_;
Expand Down

7 comments on commit 35c0286

@alexezeder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this commit introduced performance loss for byte formatting - https://alexezeder.github.io/fmt_bnchmrk/format-to-trivial-integral.html.

@alexezeder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, the results are missing from the website for some reason. Someday I will make it work properly, but for now, I can rerun benchmark for byte locally. I will do it tomorrow and post there results here.

@alexezeder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the results, for two commits - 35c0286 and c7173a3, this one and the parent one.

PC, x86_64, GCC 11.2, Clang 13

$ ./fmt_bnchmrk_format_to_trivial_gcc_35c0286cd8f1365bffbc417021e8cd23112f6c8f 
format_to_trivial_std_byte       22.4 ns         22.4 ns     31108657
$ ./fmt_bnchmrk_format_to_trivial_gcc_c7173a36a14a13f74a6734e6ae7f588340d13920 
format_to_trivial_std_byte       13.6 ns         13.6 ns     51821199
$ ./fmt_bnchmrk_format_to_trivial_clang_35c0286cd8f1365bffbc417021e8cd23112f6c8f 
format_to_trivial_std_byte       16.6 ns         16.6 ns     42683066
$ ./fmt_bnchmrk_format_to_trivial_clang_c7173a36a14a13f74a6734e6ae7f588340d13920 
format_to_trivial_std_byte       6.53 ns         6.53 ns    107598098

Raspberry, AArch64, GCC 9.3

$ ./fmt_bnchmrk_format_to_trivial # 35c0286cd8f1365bffbc417021e8cd23112f6c8f 
format_to_trivial_std_byte               245 ns          245 ns      2858389
$ ./fmt_bnchmrk_format_to_trivial # c7173a36a14a13f74a6734e6ae7f588340d13920
format_to_trivial_std_byte              93.4 ns         93.3 ns      7494567

@phprus
Copy link
Contributor

@phprus phprus commented on 35c0286 Mar 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexezeder, @vitaut,

Why format_as(std::byte b) is not constexpr and noexcept (

fmt/include/fmt/format.h

Line 2993 in 3f9b743

constexpr auto format_as(Enum e) noexcept -> underlying_t<Enum> {
)?

Maybe will help to replace:

inline auto format_as(std::byte b) -> unsigned char { return underlying(b); }

with:

constexpr auto format_as(std::byte b) noexcept -> unsigned char { return underlying(b); }

?

@alexezeder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why format_as(std::byte b) is not constexpr and noexcept (

@phprus, according to my tests this is not the case here. Both GCC and Clang on my PC generate executable that is indistinguishable from the repo version. Compiler explorer also produces the same assembly for that constexpr ... noexcept change and without it in header-only mode.

@vitaut
Copy link
Contributor Author

@vitaut vitaut commented on 35c0286 Mar 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regression should be fixed in db5b899. Thanks, @alexezeder, for catching it!

@alexezeder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the latest results for db5b899 show that the regression is fixed now.
https://alexezeder.github.io/fmt_bnchmrk/format-to-trivial-integral.html

Please sign in to comment.