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

use memchr for searching for '%' in printf format string #1984

Merged
merged 3 commits into from
Nov 8, 2020

Conversation

rimathia
Copy link
Contributor

@rimathia rimathia commented Nov 5, 2020

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

This is the same change as in the closed pull request #1853 but this time with a simple microbenchmark to argue for it. I hope this isn't considered impolite.

The change starts to be noticeable at a format string length of about 100 characters.

The microbenchmark is the same as in #1982:

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

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

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

BENCHMARK_MAIN();

The microbenchmark numbers:

master (038057e)

-------------------------------------------------------------------
Benchmark                         Time             CPU   Iterations
-------------------------------------------------------------------
long_argument/10               36.6 ns         36.6 ns     17808714
long_argument/100               188 ns          188 ns      3716840
long_argument/1000             1284 ns         1284 ns       538288
long_argument/10000           11769 ns        11767 ns        62667
long_format_string/10          39.6 ns         39.6 ns     17552922
long_format_string/100          237 ns          237 ns      2934014
long_format_string/1000        1882 ns         1882 ns       383536
long_format_string/10000      18333 ns        18328 ns        39354

branch (e662999)

-------------------------------------------------------------------
Benchmark                         Time             CPU   Iterations
-------------------------------------------------------------------
long_argument/10               39.7 ns         39.7 ns     16266435
long_argument/100               191 ns          191 ns      3609438
long_argument/1000             1304 ns         1304 ns       535373
long_argument/10000           11039 ns        11039 ns        62367
long_format_string/10          39.1 ns         39.1 ns     17829580
long_format_string/100          191 ns          191 ns      3675833
long_format_string/1000        1383 ns         1383 ns       493149
long_format_string/10000      12418 ns        12418 ns        55520

@vitaut
Copy link
Contributor

vitaut commented Nov 5, 2020

Thanks for the PR. Please fix formatting in the PR description and post benchmark results as text, not as image.

include/fmt/printf.h Outdated Show resolved Hide resolved
@rimathia
Copy link
Contributor Author

rimathia commented Nov 8, 2020

I have clicked on 'resolve comment' in the hope that this would make the red 'Changes requested' go away, I wasn't completely sure whether this is something I should do or you as the reviewer should do.

include/fmt/printf.h Outdated Show resolved Hide resolved
@vitaut vitaut merged commit 3302fd1 into fmtlib:master Nov 8, 2020
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