Sped up chrono.h formatting for cases without providing locale

[toughengineer]

Sped up chrono.h formatting (compared to the current trunk) in cases without providing locale by the amount roughly equal to three quarters of the time of

fmt::format_to(buf, FMT_COMPILE("{:%Y}"), t);

i.e. that one is sped up roughly four times.

For more elaborate format specifications and for non-compile-time cases the speedup is proportionally smaller, i.e. the speedup is not proportional to spec contents but rather is kinda a fixed amount.

[vitaut]

Thanks for the PR! Looks good, just minor comments/questions inline.

[vitaut]

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

[toughengineer]

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

[vitaut]

Yes, ternary.

[toughengineer]

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.

[vitaut]

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);
});

[toughengineer]

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.

[vitaut]

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

Please do. Thanks!

[vitaut]

Why introduce this function?

[toughengineer]

To avoid repetition in format() method.

[vitaut]

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