8000
Skip to content

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

Merged
vitaut merged 4 commits intofmtlib:masterfrom
rimathia:printf_long_format_arg
Nov 13, 2020
Merged

detail::write in one more place relevant to printf with long argument…#2016
vitaut merged 4 commits intofmtlib:masterfrom
rimathia:printf_long_format_arg

Conversation

@rimathia
Copy link
Copy Markdown
Contributor
@rimathia rimathia commented Nov 12, 2020 •
edited
Loading

…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

vitaut
vitaut requested changes Nov 13, 2020
View reviewed changes
Copy link
Copy Markdown
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.

vitaut
vitaut requested changes Nov 13, 2020
View reviewed changes
include/fmt/format.h
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
Copy Markdown
Contributor
@vitaut vitaut Nov 13, 2020

Choose a reason for hiding this comment

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

Please move these back.

include/fmt/format.h Outdated
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
Copy Markdown
Contributor
@vitaut vitaut Nov 13, 2020

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
Copy Markdown
Contributor Author
rimathia commented Nov 13, 2020

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
Copy Markdown
Contributor Author
rimathia commented Nov 13, 2020

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
Copy Markdown
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
Copy Markdown
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

Reviewers

@vitaut vitaut vitaut requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rimathia @vitaut
0