Replacing strftime with internal implementation #2544

phprus · 2021-10-13T16:01:45Z

Proposal: #2500 (comment)
Discussion: Issue #2541

All locale independent formats are implemented native with significant performance increase (С++11: 2-4 times, C++17: up to 13 times).
For locale dependent formats, strftime is used.

Draft because:

Need check ISO week/year algorithm.

Performance regression on Windows, because `ucrt` `strftime` is slow by design.

Possible solution for Windows regression: split processing into 2 way: fast path and slow path.
Slow path: unrecognized format string (non-standard extensions) or locale-dependent (except "C" locale?) formats in the format string.
Else: Fast path.
Pass the slow path formatting string to strftime or std::time_put. Fast path - internal implementation.

toughengineer · 2021-10-13T17:18:18Z

What is the goal of these changes?

phprus · 2021-10-13T17:27:55Z

Library author suggestion (#2500 (comment)) and significant performance increase (С++11: 2-4 times, C++17: up to 13 times) (https://github.com/phprus/fmt-bench/tree/optimize-tm-formatting-3).

toughengineer · 2021-10-13T18:40:20Z

So here are the results of quick and dirty tests of current trunk and you implementation at fd23701 (timings are in nanoseconds):

	%Y	%F %T	%Y-%m-%d %H:%M:%S	%a %b %d %T %Y	%a, %d %b %Y %T %z
baseline	290	436	452	570	534
baseline_compile	265	388	414	532	478
`fd23701`	40.5	66	119	647	961
fd23701_compile	16.3	29.6	50.5	544	904

The penultimate format string is a shorter form from this stackoverflow answer.
The last one is an RFC 2822 example from strftime man page.
I.e. these are not uncommon options.

Now for simple cases your implementation saves from 250 to 370 ns.
But for the more complex cases your implementation is slower from 70 to 420 ns (-ish).
This means that if a user happens to use one of the simple options, he is lucky and it runs faster. That's great!
But if the user is unlucky and uses more complex options, your implementation penalizes the user performance-wise. It runs almost twice as slow for the RFC 2822 example compared to the current trunk.

Basically you made fast cases faster, but slow cases much slower.

That's why I asked about the goal of these changes.
If it is performance improvements, which in some cases there are, what is the justification to favor simple, fast cases, and slow down complex, slower cases?

phprus · 2021-10-13T19:27:34Z

Performance test: https://github.com/phprus/fmt-bench/tree/9dc6195ad904b58394e527bc9f04ed7480764d99

macOS 10.14.6 Mojave
Apple clang version 11.0.0 (clang-1100.0.33.17)

C++11:

----------------------------------------------------------:---------------------
Benchmark                          OLD Time         CPU   :  NEW Time        CPU
----------------------------------------------------------:---------------------
FMTFormatter_Y                       425 ns      425 ns   :   45.4 ns    45.4 ns    // {:%Y}
FMTFormatterCompile_Y                426 ns      426 ns   :   59.3 ns    59.2 ns    // {:%Y}
FMTFormatter_full                    898 ns      898 ns   :    138 ns     138 ns    // {:%Y-%m-%d %H:%M:%S}
FMTFormatterCompile_full             903 ns      903 ns   :    167 ns     167 ns    // {:%Y-%m-%d %H:%M:%S}
FMTFormatter_full2                   927 ns      926 ns   :   68.6 ns    68.5 ns    // {:%F %T}
FMTFormatterCompile_full2            946 ns      946 ns   :   85.5 ns    85.5 ns    // {:%F %T}
FMTFormatter_complex_1               833 ns      832 ns   :    547 ns     547 ns    // {:%a %b %d %T %Y}
FMTFormatterCompile_complex_1        841 ns      840 ns   :    565 ns     565 ns    // {:%a %b %d %T %Y}
FMTFormatter_complex_2               983 ns      982 ns   :    862 ns     862 ns    // {:%a, %d %b %Y %T %z}
FMTFormatterCompile_complex_2       1001 ns     1001 ns   :    856 ns     856 ns    // {:%a, %d %b %Y %T %z}

C++17:

--------------------------------------------------------:-----------------------
Benchmark                          OLD Time       CPU   :   NEW Time        CPU
--------------------------------------------------------:-----------------------
FMTFormatter_Y                       440 ns    439 ns   :     46.1 ns    46.1 ns    // {:%Y}
FMTFormatterCompile_Y                389 ns    389 ns   :     14.3 ns    14.2 ns    // {:%Y}
FMTFormatter_full                    918 ns    916 ns   :      138 ns     138 ns    // {:%Y-%m-%d %H:%M:%S}
FMTFormatterCompile_full             868 ns    867 ns   :     85.8 ns    85.8 ns    // {:%Y-%m-%d %H:%M:%S}
FMTFormatter_full2                   936 ns    935 ns   :     65.9 ns    65.9 ns    // {:%F %T}
FMTFormatterCompile_full2            890 ns    889 ns   :     26.2 ns    26.2 ns    // {:%F %T}
FMTFormatter_complex_1               848 ns    847 ns   :      556 ns     556 ns    // {:%a %b %d %T %Y}
FMTFormatterCompile_complex_1        772 ns    771 ns   :      482 ns     481 ns    // {:%a %b %d %T %Y}
FMTFormatter_complex_2               993 ns    993 ns   :      963 ns     943 ns    // {:%a, %d %b %Y %T %z}
FMTFormatterCompile_complex_2        926 ns    925 ns   :      801 ns     795 ns    // {:%a, %d %b %Y %T %z}

In all cases on my MacBookPro, the presented implementation is faster.

I couldn't find an environment where strftime would be faster (especially 2 times).
Please describe your environment and provide examples of formatting strings that I can add to tests.

toughengineer · 2021-10-13T20:05:23Z

Windows 10
MSVC 16.11.4

Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)

copy paste of all runs

C++11

D:\dev\fmt-bench\out\build\x64-Release>fmt_test_old.exe
2021-10-13T22:47:16+03:00
Running fmt_test_old.exe
Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
FMTFormatter_Y                       284 ns          285 ns      2357895
FMTFormatterCompile_Y                287 ns          289 ns      2488889
FMTFormatter_full                    431 ns          430 ns      1600000
FMTFormatterCompile_full             442 ns          449 ns      1600000
FMTFormatter_full2                   415 ns          419 ns      1792000
FMTFormatterCompile_full2            403 ns          399 ns      1723077
FMTFormatter_complex_1               446 ns          449 ns      1600000
FMTFormatterCompile_complex_1        447 ns          449 ns      1600000
FMTFormatter_complex_2               527 ns          530 ns      1120000
FMTFormatterCompile_complex_2        533 ns          547 ns      1000000

D:\dev\fmt-bench\out\build\x64-Release>fmt_test_new.exe
2021-10-13T22:47:32+03:00
Running fmt_test_new.exe
Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
FMTFormatter_Y                      57.4 ns         57.2 ns     11200000
FMTFormatterCompile_Y               57.0 ns         57.8 ns     10000000
FMTFormatter_full                    138 ns          136 ns      4480000
FMTFormatterCompile_full             139 ns          138 ns      4977778
FMTFormatter_full2                  87.6 ns         87.9 ns      7466667
FMTFormatterCompile_full2           87.7 ns         87.9 ns      7466667
FMTFormatter_complex_1               607 ns          614 ns      1120000
FMTFormatterCompile_complex_1        623 ns          628 ns      1120000
FMTFormatter_complex_2               929 ns          921 ns       746667
FMTFormatterCompile_complex_2        939 ns          942 ns       746667

C++17

D:\dev\fmt-bench\out\build\x64-Release>fmt_test_old.exe
2021-10-13T22:51:33+03:00
Running fmt_test_old.exe
Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
FMTFormatter_Y                       291 ns          289 ns      2488889
FMTFormatterCompile_Y                248 ns          246 ns      2800000
FMTFormatter_full                    436 ns          433 ns      1659259
FMTFormatterCompile_full             368 ns          368 ns      1866667
FMTFormatter_full2                   406 ns          408 ns      1723077
FMTFormatterCompile_full2            351 ns          352 ns      1866667
FMTFormatter_complex_1               449 ns          445 ns      1544828
FMTFormatterCompile_complex_1        384 ns          385 ns      1866667
FMTFormatter_complex_2               527 ns          531 ns      1000000
FMTFormatterCompile_complex_2        456 ns          455 ns      1544828

D:\dev\fmt-bench\out\build\x64-Release>fmt_test_new.exe
2021-10-13T22:51:47+03:00
Running fmt_test_new.exe
Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
FMTFormatter_Y                      54.3 ns         54.7 ns     10000000
FMTFormatterCompile_Y               21.3 ns         21.3 ns     34461538
FMTFormatter_full                    152 ns          151 ns      4977778
FMTFormatterCompile_full            45.7 ns         45.5 ns     15448276
FMTFormatter_full2                  92.6 ns         92.1 ns      7466667
FMTFormatterCompile_full2           32.4 ns         32.8 ns     22400000
FMTFormatter_complex_1               621 ns          628 ns      1120000
FMTFormatterCompile_complex_1        496 ns          500 ns      1000000
FMTFormatter_complex_2               933 ns          942 ns       746667
FMTFormatterCompile_complex_2        798 ns          802 ns       896000

C++20

D:\dev\fmt-bench\out\build\x64-Release>fmt_test_old.exe
2021-10-13T22:55:29+03:00
Running fmt_test_old.exe
Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
FMTFormatter_Y                       290 ns          292 ns      2357895
FMTFormatterCompile_Y                250 ns          251 ns      2800000
FMTFormatter_full                    443 ns          449 ns      1600000
FMTFormatterCompile_full             370 ns          369 ns      1947826
FMTFormatter_full2                   411 ns          414 ns      1659259
FMTFormatterCompile_full2            345 ns          345 ns      1947826
FMTFormatter_complex_1               456 ns          446 ns      1120000
FMTFormatterCompile_complex_1        381 ns          385 ns      1866667
FMTFormatter_complex_2               551 ns          544 ns      1120000
FMTFormatterCompile_complex_2        453 ns          449 ns      1600000

D:\dev\fmt-bench\out\build\x64-Release>fmt_test_new.exe
2021-10-13T22:55:42+03:00
Running fmt_test_new.exe
Run on (8 X 3600 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
FMTFormatter_Y                      53.0 ns         53.0 ns     11200000
FMTFormatterCompile_Y               21.4 ns         21.3 ns     34461538
FMTFormatter_full                    145 ns          145 ns      5600000
FMTFormatterCompile_full            49.9 ns         50.8 ns     14451613
FMTFormatter_full2                  91.3 ns         92.8 ns      6400000
FMTFormatterCompile_full2           32.5 ns         32.2 ns     21333333
FMTFormatter_complex_1               631 ns          628 ns      1120000
FMTFormatterCompile_complex_1        501 ns          500 ns      1000000
FMTFormatter_complex_2               947 ns          942 ns       746667
FMTFormatterCompile_complex_2        828 ns          820 ns       896000

	C++11		C++17		C++20
	old	new	old	new	old	new
FMTFormatter_Y	284	57.4	291	54.3	290	53
FMTFormatterCompile_Y	287	57	248	21.3	250	21.4
FMTFormatter_full	431	138	436	152	443	145
FMTFormatterCompile_full	442	139	368	45.7	370	49.9
FMTFormatter_full2	415	87.6	406	92.6	411	91.3
FMTFormatterCompile_full2	403	87.7	351	32.4	345	32.5
FMTFormatter_complex_1	446	607	449	621	456	631
FMTFormatterCompile_complex_1	447	623	384	496	381	501
FMTFormatter_complex_2	527	929	527	933	551	947
FMTFormatterCompile_complex_2	533	939	456	798	453	828

phprus · 2021-10-13T20:12:23Z

Thanks!
What are your locale settings?

phprus · 2021-10-13T20:30:55Z

Please rerun the test with the following changes:

Replace char to wchar_t
Add #include "fmt/xchar.h" after include fmt/format.h
Remove Compile tests
Add L to format strings

toughengineer · 2021-10-13T20:33:31Z

@phprus, I don't understand what you mean by "locale settings".

Though strftime outputs something like

Sun, 31 Jan 9999 22:33:44 +0000

toughengineer · 2021-10-13T20:40:03Z

@phprus

Please rerun the test with the following changes:

Replace char to wchar_t

Add #include "fmt/xchar.h" after include fmt/format.h

Remove Compile tests

Add L to format strings

I'm not doing it because I've already spent more time on this than I would've wanted.

phprus · 2021-10-13T21:06:28Z

Windows ucrt strftime is very slow by design, because has memory allocation, convert format string from char to wchar_t and convert result string from wchar_t to char.

Replacing strftime with std::time_put<wchar_t, ...> with convert the result to char can help speed up this code on Windows. And for the C locale, may not call std::time_put, but use the built-in arrays of month names, etc.

toughengineer · 2021-10-14T11:27:36Z

Talking about slowness of strftime on Windows, it is rougly twice as fast on my slower CPU than on @phprus macbooks's faster CPU. 🤣
If there are methods to format non-trivial parts faster across platforms, this is probably a good reason to ditch strftime altogether.

vitaut · 2021-10-14T13:32:53Z

I wouldn't worry too much about Windows, we can optimize exotic formats there later (not in this PR).

…r locale independent formats

…ock, Duration> and std::tm

vitaut

Thanks for the PR! Overall looks good but please address inline comments.

include/fmt/chrono.h

test/chrono-test.cc

Requested changes

vitaut · 2021-10-16T20:20:31Z

Looks good but there is still an unneeded FormatContext template parameter and member in tm_writer. Please replace with a Char template parameter.

phprus · 2021-10-16T20:28:33Z

Done

vitaut · 2021-10-16T20:37:16Z

include/fmt/chrono.h

+};
+
+template <typename OutputIt, typename Char> class tm_writer {
+  using char_type = Char;


It's no longer needed as you can use Char directly.

vitaut · 2021-10-16T22:04:04Z

Thank you!

phprus · 2021-10-16T22:12:00Z

Tomorrow I will start porting the code to using std::time_put and std::locale.

vitaut · 2021-10-17T14:58:10Z

Looks like there is a UB in iso_year_weeks. Here's a stack trace from oss-fuzz:

/src/fmt/include/fmt/chrono.h:1428:20: runtime error: signed integer overflow: -1996866182 + -499216545 cannot be represented in type 'int'
--
  | #0 0x4d58c5 in fmt::v8::detail::tm_writer<fmt::v8::appender, char>::iso_year_weeks(int) const fmt/include/fmt/chrono.h:1428:20
  | #1 0x4d5a0e in fmt::v8::detail::tm_writer<fmt::v8::appender, char>::tm_iso_week_of_year() const fmt/include/fmt/chrono.h:1450:13
  | #2 0x4d2c99 in fmt::v8::detail::tm_writer<fmt::v8::appender, char>::on_iso_week_of_year(fmt::v8::detail::numeric_system) fmt/include/fmt/chrono.h:1622:26
  | #3 0x4d2c99 in char const* fmt::v8::detail::parse_chrono_format<char, fmt::v8::detail::tm_writer<fmt::v8::appender, char>&>(char const*, char const*, fmt::v8::detail::tm_writer<fmt::v8::appender, char>&) fmt/include/fmt/chrono.h:620:15
  | #4 0x4d1a34 in decltype(fp0.out()) fmt::v8::formatter<tm, char, void>::format<fmt::v8::basic_format_context<fmt::v8::appender, char> >(tm const&, fmt::v8::basic_format_context<fmt::v8::appender, char>&) const fmt/include/fmt/chrono.h:1768:7
  | #5 0x4d196f in void fmt::v8::detail::value<fmt::v8::basic_format_context<fmt::v8::appender, char> >::format_custom_arg<tm, fmt::v8::formatter<tm, char, void> >(void*, fmt::v8::basic_format_parse_context<char, fmt::v8::detail::error_handler>&, fmt::v8::basic_format_context<fmt::v8::appender, char>&) fmt/include/fmt/core.h:1241:22
  | #6 0x4c6c74 in fmt::v8::basic_format_arg<fmt::v8::basic_format_context<fmt::v8::appender, char> >::handle::format(fmt::v8::basic_format_parse_context<char, fmt::v8::detail::error_handler>&, fmt::v8::basic_format_context<fmt::v8::appender, char>&) const fmt/include/fmt/core.h:1503:7
  | #7 0x4c77c2 in fmt::v8::detail::custom_formatter<char>::operator()(fmt::v8::basic_format_arg<fmt::v8::basic_format_context<fmt::v8::appender, char> >::handle) const fmt/include/fmt/format.h:2197:7
  | #8 0x4c77c2 in decltype(fp(0)) fmt::v8::visit_format_arg<fmt::v8::detail::custom_formatter<char>, fmt::v8::basic_format_context<fmt::v8::appender, char> >(fmt::v8::detail::custom_formatter<char>&&, fmt::v8::basic_format_arg<fmt::v8::basic_format_context<fmt::v8::appender, char> > const&) fmt/include/fmt/core.h:1567:12
  | #9 0x4c77c2 in void fmt::v8::detail::vformat_to<char>(fmt::v8::detail::buffer<char>&, fmt::v8::basic_string_view<char>, fmt::v8::basic_format_args<fmt::v8::basic_format_context<std::__1::conditional<std::is_same<fmt::v8::type_identity<char>::type, char>::value, fmt::v8::appender, std::__1::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::type_identity<char>::type> > >::type, fmt::v8::type_identity<char>::type> >, fmt::v8::detail::locale_ref)::format_handler::on_format_specs(int, char const*, char const*) fmt/include/fmt/format.h:2938:9
  | #10 0x4c6f3c in char const* fmt::v8::detail::parse_replacement_field<char, void fmt::v8::detail::vformat_to<char>(fmt::v8::detail::buffer<char>&, fmt::v8::basic_string_view<char>, fmt::v8::basic_format_args<fmt::v8::basic_format_context<std::__1::conditional<std::is_same<fmt::v8::type_identity<char>::type, char>::value, fmt::v8::appender, std::__1::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::type_identity<char>::type> > >::type, fmt::v8::type_identity<char>::type> >, fmt::v8::detail::locale_ref)::format_handler&>(char const*, char const*, void fmt::v8::detail::vformat_to<char>(fmt::v8::detail::buffer<char>&, fmt::v8::basic_string_view<char>, fmt::v8::basic_format_args<fmt::v8::basic_format_context<std::__1::conditional<std::is_same<fmt::v8::type_identity<char>::type, char>::value, fmt::v8::appender, std::__1::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::type_identity<char>::type> > >::type, fmt::v8::type_identity<char>::type> >, fmt::v8::detail::locale_ref)::format_handler&) fmt/include/fmt/core.h:2546:23
  | #11 0x4b6311 in void fmt::v8::detail::parse_format_string<false, char, void fmt::v8::detail::vformat_to<char>(fmt::v8::detail::buffer<char>&, fmt::v8::basic_string_view<char>, fmt::v8::basic_format_args<fmt::v8::basic_format_context<std::__1::conditional<std::is_same<fmt::v8::type_identity<char>::type, char>::value, fmt::v8::appender, std::__1::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::type_identity<char>::type> > >::type, fmt::v8::type_identity<char>::type> >, fmt::v8::detail::locale_ref)::format_handler>(fmt::v8::basic_string_view<char>, void fmt::v8::detail::vformat_to<char>(fmt::v8::detail::buffer<char>&, fmt::v8::basic_string_view<char>, fmt::v8::basic_format_args<fmt::v8::basic_format_context<std::__1::conditional<std::is_same<fmt::v8::type_identity<char>::type, char>::value, fmt::v8::appender, std::__1::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::type_identity<char>::type> > >::type, fmt::v8::type_identity<char>::type> >, fmt::v8::detail::locale_ref)::format_handler&&) fmt/include/fmt/core.h:2571:21
  | #12 0x4b6311 in void fmt::v8::detail::vformat_to<char>(fmt::v8::detail::buffer<char>&, fmt::v8::basic_string_view<char>, fmt::v8::basic_format_args<fmt::v8::basic_format_context<std::__1::conditional<std::is_same<fmt::v8::type_identity<char>::type, char>::value, fmt::v8::appender, std::__1::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::type_identity<char>::type> > >::type, fmt::v8::type_identity<char>::type> >, fmt::v8::detail::locale_ref) fmt/include/fmt/format.h:2952:3
  | #13 0x4d18db in fmt::v8::appender fmt::v8::format_to<tm const&, 500ul, std::__1::allocator<char> >(fmt::v8::basic_memory_buffer<char, 500ul, std::__1::allocator<char> >&, fmt::v8::basic_format_string<char, fmt::v8::type_identity<tm const&>::type>, tm const&) fmt/include/fmt/format.h:3039:3
  | #14 0x4b5155 in void invoke_fmt<tm, long>(unsigned char const*, unsigned long) fmt/test/fuzzing/one-arg.cc:34:5
  | #15 0x4b3765 in LLVMFuzzerTestOneInput fmt/test/fuzzing/one-arg.cc:88:5
  | #16 0x43fd23 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp:0
  | #17 0x42b5d2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
  | #18 0x431085 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp:0
  | #19 0x459d82 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
  | #20 0x7fa740d900b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/libc-start.c:308:16
  | #21 0x40886d in _start

phprus · 2021-10-17T15:10:22Z

Input data is unknown?

I will prepare a patch today...

vitaut · 2021-10-17T15:33:53Z

I think it's just a large year that causes an overflow in

fmt/include/fmt/chrono.h

Line 1426 in 684e2fd

(curr_year + curr_year / 4 - curr_year / 100 + curr_year / 400) %

phprus · 2021-10-17T16:16:13Z

Cast to int64_t should be fix this UB (UB only for years over +-10^9)
#2551

vitaut · 2021-10-21T20:59:08Z

Sanitizer caught one more UB due to overflow:

#include <fmt/chrono.h>

int main() {
  char tz[] = "PDT";
  auto t = std::tm{.tm_sec = 23,
                   .tm_min = 22,
                   .tm_hour = 22,
                   .tm_mday = 15,
                   .tm_mon = 9,
                   .tm_year = 2147483638,
                   .tm_wday = 6,
                   .tm_yday = 287,
                   .tm_isdst = 1,
                   .tm_gmtoff = -25200,
                   .tm_zone = tz};

  fmt::print("{:%F}", t);
}

phprus · 2021-10-22T12:56:50Z

This is an unfixable error due to an offset of 1900 years:
https://en.cppreference.com/w/cpp/chrono/c/tm

fmt/include/fmt/chrono.h

Line 1409 in 249f03b

auto tm_year() const noexcept -> int { return 1900 + tm_.tm_year; }

In PR #2550, I added an FMT_ASSERT for the valid value ranges of the used fields of the struct tm.

If the year number overflows due to an offset of 1900 years, we can only throw an exception (we will need to check the range for each call) or output an undefined value (as now, due to a signed int overflow).

vitaut · 2021-10-22T13:17:04Z

I don't think assert is enough because we'll still get a UB when they are disabled. I suggest throwing an exception instead.

phprus · 2021-10-22T14:43:55Z

@vitaut, You're right!
I have now updated PR #2550:

Added FMT_ASSERT and extended tm validation.
Switching internal year calculations to long long and new test for tm_year overflow.

phprus marked this pull request as draft October 13, 2021 21:09

phprus mentioned this pull request Oct 14, 2021

Proposal: Switch from strftime to internal implementation for locale independent formats #2541

Closed

phprus added 16 commits October 14, 2021 23:56

New tests for all C++11 std::strftime format specifiers

187cb7a

Workarounds for implementation-defined std::strftime behavior

4310ffd

Reorder formatters

5c91419

Add format spec checker

6a26a68

New tests

c6d966a

Switch from std::strftime/std::wcsftime to internal implementation fo…

1829cc7

…r locale independent formats

Improve year formatter

c4399a7

Improve week of the year formatter

9f717a1

Improve ISO week-base-year formatter

512ec89

Fix year formatter

29775ec

Unified formatters for std::chrono::time_point<std::chrono::system_cl…

77044ae

…ock, Duration> and std::tm

New tests

0b76378

Improve performance

2eab9fc

Fix Microsoft Visual Studio 14.0 build

ac360e0

Fix error in test

98f8e33

Fix errors in ISO week-base-year formatter

8680d91

phprus marked this pull request as ready for review October 15, 2021 17:28

vitaut requested changes Oct 16, 2021

View reviewed changes

vitaut changed the title ~~Replacing strftime to internal implementation~~ Replacing strftime with internal implementation Oct 16, 2021

phprus force-pushed the optimize-tm-formatting-3 branch 3 times, most recently from 153a0c4 to c1e52a3 Compare October 16, 2021 17:44

Fix bug on '%Y' and '%C' formats with negative years

4cc64b7

Requested changes

phprus force-pushed the optimize-tm-formatting-3 branch from c1e52a3 to 4cc64b7 Compare October 16, 2021 19:39

vitaut reviewed Oct 16, 2021

View reviewed changes

Cleanup

8cdc0b3

phprus force-pushed the optimize-tm-formatting-3 branch from a8f14ec to 8cdc0b3 Compare October 16, 2021 20:41

vitaut approved these changes Oct 16, 2021

View reviewed changes

vitaut merged commit 7a604cd into fmtlib:master Oct 16, 2021

vitaut mentioned this pull request Oct 17, 2021

Improved performance of std::tm formatting #2543

Closed

phprus mentioned this pull request Oct 17, 2021

Replacing strftime with std::time_put #2550

Merged

phprus mentioned this pull request Oct 17, 2021

Fix overflow for very big years (>2*10^9) #2551

Merged

euclio mentioned this pull request Mar 15, 2022

Glibc extensions no longer supported in std::tm formatter #2811

Closed

chris-morgan mentioned this pull request Feb 23, 2023

Support %P (lowercase am/pm) glibc extension #3314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing strftime with internal implementation #2544

Replacing strftime with internal implementation #2544

phprus commented Oct 13, 2021 •

edited

Loading

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 13, 2021

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 14, 2021

vitaut commented Oct 14, 2021 •

edited

Loading

vitaut left a comment

vitaut commented Oct 16, 2021

phprus commented Oct 16, 2021

vitaut Oct 16, 2021

phprus Oct 16, 2021

vitaut commented Oct 16, 2021

phprus commented Oct 16, 2021

vitaut commented Oct 17, 2021

phprus commented Oct 17, 2021

vitaut commented Oct 17, 2021

phprus commented Oct 17, 2021

vitaut commented Oct 21, 2021

phprus commented Oct 22, 2021

vitaut commented Oct 22, 2021

phprus commented Oct 22, 2021

Replacing strftime with internal implementation #2544

Replacing strftime with internal implementation #2544

Conversation

phprus commented Oct 13, 2021 • edited Loading

Performance regression on Windows, because ucrt strftime is slow by design.

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 13, 2021

toughengineer commented Oct 13, 2021

phprus commented Oct 13, 2021

toughengineer commented Oct 14, 2021

vitaut commented Oct 14, 2021 • edited Loading

vitaut left a comment

Choose a reason for hiding this comment

vitaut commented Oct 16, 2021

phprus commented Oct 16, 2021

vitaut Oct 16, 2021

Choose a reason for hiding this comment

phprus Oct 16, 2021

Choose a reason for hiding this comment

vitaut commented Oct 16, 2021

phprus commented Oct 16, 2021

vitaut commented Oct 17, 2021

phprus commented Oct 17, 2021

vitaut commented Oct 17, 2021

phprus commented Oct 17, 2021

vitaut commented Oct 21, 2021

phprus commented Oct 22, 2021

vitaut commented Oct 22, 2021

phprus commented Oct 22, 2021

phprus commented Oct 13, 2021 •

edited

Loading

Performance regression on Windows, because `ucrt` `strftime` is slow by design.

vitaut commented Oct 14, 2021 •

edited

Loading