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.

Sign up for GitHub

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

Jump to bottom

[libc++] Add basic constant folding for std::format #107197

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[libc++] Add basic constant folding for std::format #107197

wants to merge 1 commit into from

Conversation

philnik777
Copy link
Contributor 

-------------------------------------------------------------------
Benchmark                                        old            new
-------------------------------------------------------------------
BM_format_string<char>/1                     42.1 ns        7.67 ns
BM_format_string<char>/2                     22.3 ns        3.84 ns
BM_format_string<char>/4                     10.6 ns        1.92 ns
BM_format_string<char>/8                     5.31 ns       0.815 ns
BM_format_string<char>/16                    2.79 ns       0.480 ns
BM_format_string<char>/32                    1.63 ns       0.550 ns
BM_format_string<char>/64                   0.782 ns       0.276 ns
BM_format_string<char>/128                  0.397 ns       0.145 ns
BM_format_string<char>/256                  0.211 ns       0.066 ns
BM_format_string<char>/512                  0.154 ns       0.035 ns
BM_format_string<char>/1024                 0.146 ns       0.021 ns
BM_format_string<char>/2048                 0.125 ns       0.033 ns
BM_format_string<char>/4096                 0.097 ns       0.016 ns
BM_format_string<char>/8192                 0.077 ns       0.012 ns
BM_format_string<char>/16384                0.066 ns       0.010 ns
BM_format_string<char>/32768                0.062 ns       0.016 ns
BM_format_string<char>/65536                0.062 ns       0.016 ns
BM_format_string<char>/131072               0.443 ns       0.015 ns
BM_format_string<char>/262144               0.629 ns       0.017 ns
BM_format_string<char>/524288               0.715 ns       0.020 ns
BM_format_string<char>/1048576              0.757 ns       0.020 ns
BM_format_string<wchar_t>/1                  38.8 ns        34.0 ns
BM_format_string<wchar_t>/2                  19.4 ns        16.9 ns
BM_format_string<wchar_t>/4                  9.88 ns        8.45 ns
BM_format_string<wchar_t>/8                  6.30 ns        6.47 ns
BM_format_string<wchar_t>/16                 3.11 ns        3.21 ns
BM_format_string<wchar_t>/32                 1.60 ns        1.63 ns
BM_format_string<wchar_t>/64                0.899 ns       0.925 ns
BM_format_string<wchar_t>/128               0.676 ns       0.693 ns
BM_format_string<wchar_t>/256               0.658 ns       0.685 ns
BM_format_string<wchar_t>/512               0.556 ns       0.531 ns
BM_format_string<wchar_t>/1024              0.428 ns       0.402 ns
BM_format_string<wchar_t>/2048              0.328 ns       0.319 ns
BM_format_string<wchar_t>/4096              0.276 ns       0.274 ns
BM_format_string<wchar_t>/8192              0.252 ns       0.251 ns
BM_format_string<wchar_t>/16384             0.248 ns       0.246 ns
BM_format_string<wchar_t>/32768             0.229 ns       0.232 ns
BM_format_string<wchar_t>/65536             0.248 ns       0.246 ns
BM_format_string<wchar_t>/131072            0.250 ns       0.240 ns
BM_format_string<wchar_t>/262144             3.03 ns        3.03 ns
BM_format_string<wchar_t>/524288             3.14 ns        3.15 ns
BM_format_string<wchar_t>/1048576            3.60 ns        3.61 ns
BM_string_without_formatting<char>           32.2 ns       0.470 ns
BM_string_without_formatting<wchar_t>        38.8 ns        10.2 ns

@philnik777 philnik777 requested a review from a team as a code owner September 4, 2024 08:13
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2024

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes 
-------------------------------------------------------------------
Benchmark                                        old            new
-------------------------------------------------------------------
BM_format_string&lt;char&gt;/1                     42.1 ns        7.67 ns
BM_format_string&lt;char&gt;/2                     22.3 ns        3.84 ns
BM_format_string&lt;char&gt;/4                     10.6 ns        1.92 ns
BM_format_string&lt;char&gt;/8                     5.31 ns       0.815 ns
BM_format_string&lt;char&gt;/16                    2.79 ns       0.480 ns
BM_format_string&lt;char&gt;/32                    1.63 ns       0.550 ns
BM_format_string&lt;char&gt;/64                   0.782 ns       0.276 ns
BM_format_string&lt;char&gt;/128                  0.397 ns       0.145 ns
BM_format_string&lt;char&gt;/256                  0.211 ns       0.066 ns
BM_format_string&lt;char&gt;/512                  0.154 ns       0.035 ns
BM_format_string&lt;char&gt;/1024                 0.146 ns       0.021 ns
BM_format_string&lt;char&gt;/2048                 0.125 ns       0.033 ns
BM_format_string&lt;char&gt;/4096                 0.097 ns       0.016 ns
BM_format_string&lt;char&gt;/8192                 0.077 ns       0.012 ns
BM_format_string&lt;char&gt;/16384                0.066 ns       0.010 ns
BM_format_string&lt;char&gt;/32768                0.062 ns       0.016 ns
BM_format_string&lt;char&gt;/65536                0.062 ns       0.016 ns
BM_format_string&lt;char&gt;/131072               0.443 ns       0.015 ns
BM_format_string&lt;char&gt;/262144               0.629 ns       0.017 ns
BM_format_string&lt;char&gt;/524288               0.715 ns       0.020 ns
BM_format_string&lt;char&gt;/1048576              0.757 ns       0.020 ns
BM_format_string&lt;wchar_t&gt;/1                  38.8 ns        34.0 ns
BM_format_string&lt;wchar_t&gt;/2                  19.4 ns        16.9 ns
BM_format_string&lt;wchar_t&gt;/4                  9.88 ns        8.45 ns
BM_format_string&lt;wchar_t&gt;/8                  6.30 ns        6.47 ns
BM_format_string&lt;wchar_t&gt;/16                 3.11 ns        3.21 ns
BM_format_string&lt;wchar_t&gt;/32                 1.60 ns        1.63 ns
BM_format_string&lt;wchar_t&gt;/64                0.899 ns       0.925 ns
BM_format_string&lt;wchar_t&gt;/128               0.676 ns       0.693 ns
BM_format_string&lt;wchar_t&gt;/256               0.658 ns       0.685 ns
BM_format_string&lt;wchar_t&gt;/512               0.556 ns       0.531 ns
BM_format_string&lt;wchar_t&gt;/1024              0.428 ns       0.402 ns
BM_format_string&lt;wchar_t&gt;/2048              0.328 ns       0.319 ns
BM_format_string&lt;wchar_t&gt;/4096              0.276 ns       0.274 ns
BM_format_string&lt;wchar_t&gt;/8192              0.252 ns       0.251 ns
BM_format_string&lt;wchar_t&gt;/16384             0.248 ns       0.246 ns
BM_format_string&lt;wchar_t&gt;/32768             0.229 ns       0.232 ns
BM_format_string&lt;wchar_t&gt;/65536             0.248 ns       0.246 ns
BM_format_string&lt;wchar_t&gt;/131072            0.250 ns       0.240 ns
BM_format_string&lt;wchar_t&gt;/262144             3.03 ns        3.03 ns
BM_format_string&lt;wchar_t&gt;/524288             3.14 ns        3.15 ns
BM_format_string&lt;wchar_t&gt;/1048576            3.60 ns        3.61 ns
BM_string_without_formatting&lt;char&gt;           32.2 ns       0.470 ns
BM_string_without_formatting&lt;wchar_t&gt;        38.8 ns        10.2 ns

Full diff: https://github.com/llvm/llvm-project/pull/107197.diff

2 Files Affected:

  • (modified) libcxx/include/__format/format_functions.h (+42-6)
  • (modified) libcxx/test/benchmarks/format.bench.cpp (+9)
diff --git a/libcxx/include/__format/format_functions.h b/libcxx/include/__format/format_functions.h
index 1518ab5768d243..dbb0b7bfcb12b4 100644
--- a/libcxx/include/__format/format_functions.h
+++ b/libcxx/include/__format/format_functions.h
@@ -11,6 +11,7 @@
 #define _LIBCPP___FORMAT_FORMAT_FUNCTIONS
 
 #include <__algorithm/clamp.h>
+#include <__algorithm/ranges_find_first_of.h>
 #include <__concepts/convertible_to.h>
 #include <__concepts/same_as.h>
 #include <__config>
@@ -448,13 +449,44 @@ format_to(_OutIt __out_it, wformat_string<_Args...> __fmt, _Args&&... __args) {
 }
 #  endif
 
+template <class _CharT>
+[[nodiscard]] optional<basic_string<_CharT>> __try_constant_folding(
+    basic_string_view<_CharT> __fmt,
+    basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) {
+  return nullopt;
+  if (bool __is_identity = std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end();
+      __builtin_constant_p(__is_identity) && __is_identity)
+    return basic_string<_CharT>{__fmt};
+
+  if (auto __only_first_arg = __fmt == _LIBCPP_STATICALLY_WIDEN(_CharT, "{}");
+      __builtin_constant_p(__only_first_arg) && __only_first_arg) {
+    if (auto __arg = __args.get(0); __builtin_constant_p(__arg.__type_)) {
+      return std::__visit_format_arg(
+          []<class _Tp>(_Tp&& __argument) -> optional<basic_string<_CharT>> {
+            if constexpr (is_same_v<remove_cvref_t<_Tp>, basic_string_view<_CharT>>) {
+              return basic_string<_CharT>{__argument};
+            } else {
+              return nullopt;
+            }
+          },
+          __arg);
+    }
+  }
+
+  return nullopt;
+}
+
 // TODO FMT This needs to be a template or std::to_chars(floating-point) availability markup
 // fires too eagerly, see http://llvm.org/PR61563.
 template <class = void>
 [[nodiscard]] _LIBCPP_ALWAYS_INLINE inline _LIBCPP_HIDE_FROM_ABI string vformat(string_view __fmt, format_args __args) {
-  string __res;
-  std::vformat_to(std::back_inserter(__res), __fmt, __args);
-  return __res;
+  return std::__try_constant_folding(__fmt, __args)
+      .or_else([&]() -> optional<string> {
+        string __res;
+        std::vformat_to(std::back_inserter(__res), __fmt, __args);
+        return __res;
+      })
+      .value();
 }
 
 #  ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
@@ -463,9 +495,13 @@ template <class = void>
 template <class = void>
 [[nodiscard]] _LIBCPP_ALWAYS_INLINE inline _LIBCPP_HIDE_FROM_ABI wstring
 vformat(wstring_view __fmt, wformat_args __args) {
-  wstring __res;
-  std::vformat_to(std::back_inserter(__res), __fmt, __args);
-  return __res;
+  return std::__try_constant_folding(__fmt, __args)
+      .or_else([&]() -> optional<wstring> {
+        wstring __res;
+        std::vformat_to(std::back_inserter(__res), __fmt, __args);
+        return __res;
+      })
+      .value();
 }
 #  endif
 
diff --git a/libcxx/test/benchmarks/format.bench.cpp b/libcxx/test/benchmarks/format.bench.cpp
index 17eaccb18ee1c4..5632aa5b9810f3 100644
--- a/libcxx/test/benchmarks/format.bench.cpp
+++ b/libcxx/test/benchmarks/format.bench.cpp
@@ -28,4 +28,13 @@ static void BM_format_string(benchmark::State& state) {
 BENCHMARK(BM_format_string<char>)->RangeMultiplier(2)->Range(1, 1 << 20);
 BENCHMARK(BM_format_string<wchar_t>)->RangeMultiplier(2)->Range(1, 1 << 20);
 
+template <class CharT>
+static void BM_string_without_formatting(benchmark::State& state) {
+  for (auto _ : state) {
+    benchmark::DoNotOptimize(std::format(CSTR("Hello, World!")));
+  }
+}
+BENCHMARK(BM_string_without_formatting<char>);
+BENCHMARK(BM_string_without_formatting<wchar_t>);
+
 BENCHMARK_MAIN();

ldionne
ldionne reviewed Sep 12, 2024
View reviewed changes
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

At first glance I think this makes sense. I'm unsure how much this is actually buying us cause this pattern might be really really infrequent. However as @philnik777 pointed out just now this might be common when format is used through std::print.

One general thing I'd like to mention is that we should be mindful of making our implementations more heavyweight for these kinds of optimizations. I think they are great and most of the time worth it, but we should keep in mind that we're adding more dependencies and more "code" for the compiler to churn through. Not a problem with this patch, but we should keep this in our heads as we work on these optimizations to make sure we don't lose sight of the big picture.

ldionne
ldionne reviewed Nov 6, 2024
View reviewed changes
@philnik777 philnik777 force-pushed the constant_fold_format branch from 3c4cb14 to 657a6cb Compare January 27, 2025 16:13
@ldionne ldionne added the pending-ci Merging the PR is only pending completion of CI label Jan 27, 2025
@philnik777 philnik777 force-pushed the constant_fold_format branch from 657a6cb to 9be4e43 Compare January 30, 2025 20:18
mordante
mordante requested changes Feb 2, 2025
View reviewed changes
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Sorry I seem to have missed this patch before,
Thanks for working on this!

I have a hard time to review the patch

  • the patch does not describe what it does, there are only benchmarks
  • the code has no comment.

Did you benchmark what to overhead for the types are?

libcxx/include/__format/format_functions.h Outdated
basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) {
// [[gnu::pure]] is added to ensure that the compiler always knows that this call can be eliminated.
if (bool __is_identity =
[&] [[__gnu__::__pure__]] { return std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end(); }();
Copy link
Member

Choose a reason for hiding this comment

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

Why testing against '{' instead of _Chart('{')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for one over the other?

Copy link
Member

Choose a reason for hiding this comment

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

In format we typically do the latter, so it reads very odd to do things differently here. I'm even a bit surprised this compiles since you want to compare wchar_t against char.

philnik777
philnik777 commented Feb 2, 2025
View reviewed changes
Copy link
Contributor Author

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

Did you benchmark what to overhead for the types are?

I'm not sure what you're trying to ask.

libcxx/include/__format/format_functions.h Outdated
basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) {
// [[gnu::pure]] is added to ensure that the compiler always knows that this call can be eliminated.
if (bool __is_identity =
[&] [[__gnu__::__pure__]] { return std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end(); }();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for one over the other?

@philnik777 philnik777 force-pushed the constant_fold_format branch from 9be4e43 to 0db3132 Compare February 3, 2025 19:23
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command: 
git-clang-format --diff 956cfa69b153a0e798060f67e713790eeefebc04 0db3132b818ba1fd1f8d5b5d684e5170d31375df --extensions ,h,cpp -- libcxx/include/__config libcxx/include/__format/format_functions.h libcxx/test/benchmarks/format/format.bench.cpp
View the diff from clang-format here. 
diff --git a/libcxx/include/__format/format_functions.h b/libcxx/include/__format/format_functions.h
index fd71f53b51..43d1a6ce1f 100644
--- a/libcxx/include/__format/format_functions.h
+++ b/libcxx/include/__format/format_functions.h
@@ -456,7 +456,6 @@ template <class _CharT>
 [[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<basic_string<_CharT>> __try_constant_folding_format(
     basic_string_view<_CharT> __fmt,
     basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) {
-
   // Fold strings not containing '{' or '}' to just return the string
   if (bool __is_identity = [&] [[__gnu__::__pure__]] // Make sure the compiler knows this call can be eliminated
       { return std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end(); }();

@ldionne ldionne removed the pending-ci Merging the PR is only pending completion of CI label Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mordante mordante mordante requested changes

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@ldionne ldionne ldionne approved these changes

Assignees

@mordante mordante

Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@philnik777 @llvmbot @ldionne @mordante @frederick-vs-ja