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.

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

detail::write in one more place relevant to printf with long argument… #2016

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

rimathia
Copy link
Contributor

@rimathia rimathia commented Nov 12, 2020

…s, requires moving the definition of detail::write up in the file

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

The benchmark just looks at the scaling for a long string argument:

#include <benchmark/benchmark.h>
#include <fmt/printf.h

void long_printf_format_arg(benchmark::State& state) {
  std::string format_string = "%s";
  std::string message = std::string(state.range(0), 'A');
  for (auto _ : state) {
    benchmark::DoNotOptimize(fmt::sprintf(format_string, message));
  };
}
BENCHMARK(long_printf_format_arg)->Arg(10)->Arg(100)->Arg(1000)->Arg(10000);

void long_fmt_format_arg(benchmark::State& state) {
  std::string format_string = "{}";
  std::string message = std::string(state.range(0), 'A');
  for (auto _ : state) {
    benchmark::DoNotOptimize(fmt::format(format_string, message));
  };
}
BENCHMARK(long_fmt_format_arg)->Arg(10)->Arg(100)->Arg(1000)->Arg(10000);

BENCHMARK_MAIN();

Timings on main (986fa00):

-----------------------------------------------------------------------
Benchmark                             Time             CPU   Iterations
-----------------------------------------------------------------------
long_printf_format_arg/10          38.9 ns         38.9 ns     17873649
long_printf_format_arg/100          192 ns          192 ns      3640240
long_printf_format_arg/1000        1297 ns         1297 ns       526502
long_printf_format_arg/10000      11060 ns        11059 ns        62581
long_fmt_format_arg/10             15.7 ns         15.7 ns     44365010
long_fmt_format_arg/100            75.3 ns         75.3 ns      9133969
long_fmt_format_arg/1000            121 ns          121 ns      5455920
long_fmt_format_arg/10000           275 ns          275 ns      2542404

Timings on the branch (6684314):

-----------------------------------------------------------------------
Benchmark                             Time             CPU   Iterations
-----------------------------------------------------------------------
long_printf_format_arg/10          37.0 ns         37.0 ns     18809721
long_printf_format_arg/100         94.1 ns         94.1 ns      7307423
long_printf_format_arg/1000         250 ns          250 ns      2756264
long_printf_format_arg/10000        443 ns          443 ns      1569507
long_fmt_format_arg/10             15.8 ns         15.8 ns     43821209
long_fmt_format_arg/100            75.8 ns         75.8 ns      9125515
long_fmt_format_arg/1000            118 ns          118 ns      5865348
long_fmt_format_arg/10000           294 ns          294 ns      2473629

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This is a nice optimization but instead of replacing copy_str with write, please move the buffer handling logic from https://github.com/fmtlib/fmt/blob/master/include/fmt/format.h#L2058-L2063 to copy_str.

Comment on lines 2045 to 2064
template <typename Char, typename OutputIt>
OutputIt write(OutputIt out, monostate) {
FMT_ASSERT(false, "");
return out;
}

template <typename Char, typename OutputIt,
FMT_ENABLE_IF(!std::is_same<Char, char>::value)>
OutputIt write(OutputIt out, string_view value) {
auto it = reserve(out, value.size());
it = copy_str<Char>(value.begin(), value.end(), it);
return base_iterator(out, it);
}

template <typename Char, typename OutputIt>
OutputIt write(OutputIt out, basic_string_view<Char> value) {
auto it = reserve(out, value.size());
it = copy_str<Char>(value.begin(), value.end(), it);
return base_iterator(out, it);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move these back.

Comment on lines 2066 to 2071
template <typename Char>
buffer_appender<Char> write(buffer_appender<Char> out,
basic_string_view<Char> value) {
get_container(out).append(value.begin(), value.end());
return out;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This overload can be removed now.

@rimathia
Copy link
Contributor Author

According to git diff origin/master...printf_long_format_arg the shifts of detail::write aren't part of the updated pull request anymore, please let me know if I'm confused here, then I'll just make a new branch for a pull request with a clean history

@rimathia
Copy link
Contributor Author

The present specialization misses the opportunity to use buffer::append when converting between char_8 and char, but that seemed a bit tricky to get to work. Am I correct in assuming that this isn't a particularly relevant use case?

@vitaut
Copy link
Contributor

vitaut commented Nov 13, 2020

Am I correct in assuming that this isn't a particularly relevant use case?

Yes, char8_t should be avoided anyway.

@vitaut vitaut merged commit b268f88 into fmtlib:master Nov 13, 2020
@vitaut
Copy link
Contributor

vitaut commented Nov 13, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants