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.

Sign up for GitHub

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

Sped up chrono.h formatting for cases without providing locale #2576

Merged
merged 1 commit into from
Nov 7, 2021
Merged

Sped up chrono.h formatting for cases without providing locale #2576

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 36 additions & 19 deletions include/fmt/chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,12 +309,17 @@ auto write(OutputIt out, const std::tm& time, const std::locale& loc,
return std::copy(str.begin(), str.end(), out);
}

inline const std::locale& get_classic_locale() {
static const auto& locale = std::locale::classic();
return locale;
}

template <typename Char, typename OutputIt,
FMT_ENABLE_IF(std::is_same<Char, char>::value)>
auto write(OutputIt out, const std::tm& time, const std::locale& loc,
char format, char modifier = 0) -> OutputIt {
auto str = do_write<char>(time, loc, format, modifier);
if (detail::is_utf8() && loc != std::locale::classic()) {
if (detail::is_utf8() && loc != get_classic_locale()) {
// char16_t and char32_t codecvts are broken in MSVC (linkage errors) and
// gcc-4.
#if FMT_MSC_VER != 0 || \
Expand Down Expand Up @@ -1057,9 +1062,14 @@ struct chrono_formatter {

void format_localized(const tm& time, char format, char modifier = 0) {
if (isnan(val)) return write_nan();
const auto& loc = localized ? context.locale().template get<std::locale>()
: std::locale::classic();
out = detail::write<char_type>(out, time, loc, format, modifier);
if (localized) {
out = detail::write<char_type>(
out, time, context.locale().template get<std::locale>(), format,
modifier);
} else {
out = detail::write<char_type>(out, time, get_classic_locale(), format,
modifier);
}
Comment on lines +1065 to +1072
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below: let's keep the conditional expression to avoid repetition.

Copy link
Contributor Author

@toughengineer toughengineer Nov 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean by "conditional expression"? Ternary ?:?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, ternary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it looks like this in the trunk:

const auto& loc = localized ? context.locale().template get<std::locale>()
                            : std::locale::classic();

context.locale().get() returns locale object by value so this expression results in a temporary object.

Since one of the subexpressions results in a temporary object, the whole conditional operator

localized ? context.locale().template get<std::locale>() : std::locale::classic()

results in a temporary object. The resulting temporary is then bound to a const reference.

When the condition is false (e.g. localized==false or locale reference is empty and therefore bool{context.locale()}==false) the last subexpression is evaluated: std::locale::classic() in this case.

Since the conditional operator results in a temporary, the locale object classic() returns reference to is copy constructed into a temporary object, and then destructed at the end of the scope. Copy construction and destruction of locale objects are relatively expensive operations because they need to increment and decrement a bunch of reference counters of locale facets. This is significant compared to work done for simple format specifications like %Y.

I changed it so these expressions are evaluated in separate scopes. Depending on the condition, context.locale().get() results in a temporary which is bound to a function parameter, and a reference returned by std::locale::classic() is passed directly as function parameter without introducing a temporary.

Please let me know if you want me to back all of this up by quotes from and/or links to the C++ standard and/or cppreference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for the detailed explanation. One thing that concerns me is that this change can be easily undone by accident with a seemingly innocent refactor. I suggest moving this optimized locale handling logic into a separate function:

template <typename FormatContext, typename F>
void with_locale(bool localized, FormatContext& ctx, F f) {
  if (localized) f(ctx.locale().template get<std::locale>());
  else f(get_classic_locale());
}

and adding a short comment explaining why it is needed.

Then the usage can be something like

with_locale(localized, context, [&](std::locale&) {
  out = detail::write<char_type>(out, time, locale, format, modifier);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One other variant can be to lazily construct the requested locale within the context object and return const reference to it from context.locale().

Anyway I think such refactoring is out of the scope of this PR.

I can do something like you suggested in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do something like you suggested in the next PR.

Please do. Thanks!

}

void on_text(const char_type* begin, const char_type* end) {
Expand Down Expand Up @@ -1226,9 +1236,11 @@ template <typename Char> struct formatter<weekday, Char> {
auto format(weekday wd, FormatContext& ctx) const -> decltype(ctx.out()) {
auto time = std::tm();
time.tm_wday = static_cast<int>(wd.c_encoding());
const auto& loc = localized ? ctx.locale().template get<std::locale>()
: std::locale::classic();
return detail::write<Char>(ctx.out(), time, loc, 'a');
if (localized)
return detail::write<Char>(ctx.out(), time,
ctx.locale().template get<std::locale>(), 'a');
return detail::write<Char>(ctx.out(), time, detail::get_classic_locale(),
'a');
}
};

Expand Down Expand Up @@ -1543,7 +1555,7 @@ template <typename OutputIt, typename Char> class tm_writer {
public:
explicit tm_writer(const std::locale& loc, OutputIt out, const std::tm& tm)
: loc_(loc),
is_classic_(loc_ == std::locale::classic()),
is_classic_(loc_ == get_classic_locale()),
out_(out),
tm_(tm) {}

Expand Down Expand Up @@ -1822,6 +1834,18 @@ template <typename Char> struct formatter<std::tm, Char> {
return end;
}

template <typename It>
It do_format(It out, const std::tm& tm, const std::locale& loc) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce this function?

Copy link
Contributor Author

@toughengineer toughengineer Nov 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To avoid repetition in format() method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping ternary should avoid the repetition and introducing a new helper funtion.

auto w = detail::tm_writer<It, Char>(loc, out, tm);
if (spec_ == spec::year_month_day)
w.on_iso_date();
else if (spec_ == spec::hh_mm_ss)
w.on_iso_time();
else
detail::parse_chrono_format(specs.begin(), specs.end(), w);
return w.out();
}

public:
template <typename ParseContext>
FMT_CONSTEXPR auto parse(ParseContext& ctx) -> decltype(ctx.begin()) {
Expand All @@ -1831,17 +1855,10 @@ template <typename Char> struct formatter<std::tm, Char> {
template <typename FormatContext>
auto format(const std::tm& tm, FormatContext& ctx) const
-> decltype(ctx.out()) {
const auto& loc_ref = ctx.locale();
const auto& loc =
loc_ref ? loc_ref.template get<std::locale>() : std::locale::classic();
auto w = detail::tm_writer<decltype(ctx.out()), Char>(loc, ctx.out(), tm);
if (spec_ == spec::year_month_day)
w.on_iso_date();
else if (spec_ == spec::hh_mm_ss)
w.on_iso_time();
else
detail::parse_chrono_format(specs.begin(), specs.end(), w);
return w.out();
if (const auto& loc_ref = ctx.locale())
return this->do_format(ctx.out(), tm,
loc_ref.template get<std::locale>());
return this->do_format(ctx.out(), tm, detail::get_classic_locale());
}
};

Expand Down