Optimize tm formatting (Non C-locales and %Z) #2617

phprus · 2021-11-26T18:51:01Z

Replaced std::basic_ostringstream to std::basic_ostream with fmt::detail::formatbuf.

Benchmark: https://github.com/phprus/fmt-bench/tree/optimize-tm-formatting-6

vitaut · 2021-11-26T20:30:04Z

Thanks for the PR. I wonder why strftime is so much faster in the benchmark?

phprus · 2021-11-27T08:04:51Z

glibc and BSD libc has a tm.tm_zone field. I will add using of the tm.tm_zone to this PR.
But why ostringstream is so slower I don't know. I will do this research on Monday.

phprus · 2021-11-27T13:02:24Z

@vitaut,
glibc strftime does not use locales.
libstdc++ uses strftime_l and strftime_l uses locales.

New algorithm for Z format is implemented. Benchmark is updated.

vitaut

Thanks for the PR!

include/fmt/format.h

vitaut · 2021-12-03T17:15:58Z

include/fmt/format.h

@@ -38,6 +38,7 @@
 #include <limits>        // std::numeric_limits
 #include <memory>        // std::uninitialized_copy
 #include <stdexcept>     // std::runtime_error
+#include <streambuf>     // std::basic_streambuf


Adding a dependency on streambuf is undesirable esp. since formatbuf is not even used in format.h. Let's drop it by making formatbuf parameterized on streambuf.

include/fmt/chrono.h

vitaut · 2021-12-03T17:26:48Z

include/fmt/chrono.h

-inline auto do_write(const std::tm& time, const std::locale& loc, char format,
-                     char modifier) -> std::basic_string<Char> {
-  auto&& os = std::basic_ostringstream<Char>();
+inline void do_write(buffer<Char>& buf, const std::tm& time,


nit: I think we can drop do_ here and below because this prefix is only used in cases with identical signatures. Here signature is specific enough.

Done.
[do_]write(buffer<Char>& buf, conflicts with write(OutputIt out

vitaut · 2021-12-03T17:30:31Z

include/fmt/chrono.h

+    constexpr size_t unit_buf_size = 32;
+    code_unit unit_buf[unit_buf_size] = {};


Let's move buffer management to [do_]write_codecvt by making it return a struct, something like:

struct codecvt_result { code_unit buf[32]; code_unit* end; };

Why? write_codecvt does not depend on the the output buffer size. So I moved the buffer management out of write_codecvt.

Because otherwise it has to be done by the callers effectively duplicating the logic.

end value is dependent of buf.
Returning codecvt_result from a function corrupts end.

phprus · 2021-12-03T19:19:37Z

@vitaut, PR updated.

phprus · 2021-12-03T19:21:53Z

Unexpected runtime error: https://github.com/phprus/fmt/runs/4412073708?check_suite_focus=true
I convert PR to draft...

include/fmt/format.h

…rmatbuf

phprus · 2021-12-03T20:40:33Z

@vitaut, PR updated. Error on Windows is fixed.

vitaut

In general looks good, just a few more comments.

include/fmt/chrono.h

vitaut · 2021-12-04T14:29:13Z

include/fmt/chrono.h

+};
+
+template <typename CodeUnit>
+inline void write_codecvt(codecvt_result<CodeUnit>& out, string_view in_buf,


Why not return codecvt_result instead of passing it as an output parameter?

Also inline is probably not needed here.

codecvt_result.end is depends on codecvt_result.buf
Returning codecvt_result from a function may result in codecvt_result being copied and end being corrupted.
It is possible to replace the pointer end with the length len, but this does not guarantee that there is no copying.

Makes sense, let's leave as out parameter then.

vitaut · 2021-12-04T14:35:35Z

include/fmt/chrono.h

+  auto&& buf = basic_memory_buffer<Char>();
+  buf.append(sv.begin(), sv.end());
+  return write_char_buffer(out, buf, loc);


I think it's better to replace write_char_buffer with a function that takes string_view instead of a buffer to avoid copy.

vitaut · 2021-12-04T14:37:44Z

include/fmt/chrono.h

-    for (code_unit* p = buf; p != to_next; ++p) {
+    codecvt_result<code_unit> unit;
+    write_codecvt(unit, string_view(buf.data(), buf.size()), loc);
+    buf.clear();


I think this function should use a new buffer instead of reusing the one passed externally. This will also allow to pass input as a string_view suggested below.

vitaut · 2021-12-04T19:02:36Z

Thank you!

phprus changed the title ~~Optimize tm formatting (Non C-locales)~~ Optimize tm formatting (Non C-locales and %Z) Nov 26, 2021

phprus marked this pull request as draft November 27, 2021 08:36

phprus marked this pull request as ready for review November 27, 2021 13:02

phprus force-pushed the optimize-tm-formatting-6 branch from ce6f4f2 to 16faf54 Compare November 27, 2021 16:13

vitaut requested changes Dec 3, 2021

View reviewed changes

vitaut reviewed Dec 3, 2021

View reviewed changes

phprus force-pushed the optimize-tm-formatting-6 branch 2 times, most recently from 387d007 to 2cf2bd1 Compare December 3, 2021 19:10

phprus marked this pull request as draft December 3, 2021 19:22

vitaut reviewed Dec 3, 2021

View reviewed changes

include/fmt/format.h Outdated Show resolved Hide resolved

phprus force-pushed the optimize-tm-formatting-6 branch from 2cf2bd1 to 7a6fefc Compare December 3, 2021 20:04

phprus added 2 commits December 4, 2021 01:21

Move fmt::detail::formatbuf to format.h

4e34d4d

Replace std::basic_ostringstream to std::basic_ostream with custom fo…

e10e685

…rmatbuf

phprus force-pushed the optimize-tm-formatting-6 branch from 7a6fefc to ecd81cf Compare December 3, 2021 20:32

phprus marked this pull request as ready for review December 3, 2021 20:40

vitaut requested changes Dec 4, 2021

View reviewed changes

Use tm.tm_zone

608a9a3

phprus force-pushed the optimize-tm-formatting-6 branch from ecd81cf to 608a9a3 Compare December 4, 2021 17:40

vitaut approved these changes Dec 4, 2021

View reviewed changes

vitaut merged commit c240d98 into fmtlib:master Dec 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize tm formatting (Non C-locales and %Z) #2617

Optimize tm formatting (Non C-locales and %Z) #2617

phprus commented Nov 26, 2021

vitaut commented Nov 26, 2021

phprus commented Nov 27, 2021

phprus commented Nov 27, 2021

vitaut left a comment

vitaut Dec 3, 2021

phprus Dec 3, 2021

vitaut Dec 3, 2021

phprus Dec 3, 2021 •

edited

Loading

vitaut Dec 3, 2021

phprus Dec 3, 2021

vitaut Dec 3, 2021

phprus Dec 3, 2021

phprus Dec 3, 2021

phprus commented Dec 3, 2021

phprus commented Dec 3, 2021

phprus commented Dec 3, 2021

vitaut left a comment

vitaut Dec 4, 2021

phprus Dec 4, 2021

vitaut Dec 4, 2021

vitaut Dec 4, 2021

phprus Dec 4, 2021

vitaut Dec 4, 2021

phprus Dec 4, 2021

vitaut commented Dec 4, 2021

		constexpr size_t unit_buf_size = 32;
		code_unit unit_buf[unit_buf_size] = {};

Optimize tm formatting (Non C-locales and %Z) #2617

Optimize tm formatting (Non C-locales and %Z) #2617

Conversation

phprus commented Nov 26, 2021

vitaut commented Nov 26, 2021

phprus commented Nov 27, 2021

phprus commented Nov 27, 2021

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phprus Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phprus commented Dec 3, 2021

phprus commented Dec 3, 2021

phprus commented Dec 3, 2021

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut commented Dec 4, 2021

phprus Dec 3, 2021 •

edited

Loading