[libcxx-commits] [PATCH] D128600: [libc++][mingw] Remove setlocale from snprintf_l

[Alvin Wong via Phabricator via libcxx-commits]

alvinhochun created this revision.
Herald added a subscriber: mstorsjo.
Herald added a project: All.
alvinhochun published this revision for review.
alvinhochun added reviewers: mstorsjo, EricWF.
alvinhochun added a subscriber: thomasanderson.
alvinhochun added a comment.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

This is an alternative of D59525 <https://reviews.llvm.org/D59525> and D59727 <https://reviews.llvm.org/D59727> for mingw. I think this implementation using `_vsnprintf_s_l` and `_vscprintf_l` should replicate the expected behaviour of `snprintf_l`, with NULL terminator and such. @mstorsjo should be able to confirm whether These two functions are available on both UCRT and MSVCRT configuration of mingw.

In case of insufficient buffer, this implementation has to process the format string twice, but doing this is still much faster than having to call setlocale multiple times.

I have not run any of the libcxx test suites on Windows and I don't have a build setup to do so.

I did test the same benchmark in https://github.com/llvm/llvm-project/issues/56202#issuecomment-1166229891, first built using the vanilla `llvm-mingw-20220323-ucrt-x86_64` native toolchain and ran, then ran with a replacement `libc++.dll` built from `llvmorg-14.0.0` with this patch applied. The performance difference is significant.

Without patch:
--------------

| ns/op                 | op/s                  | err%      | total       | benchmark                           |
| --------------------: | --------------------: | --------: | ----------: | :----------                         |
| 89.32                 | 11,195,659.27         | 0.8%      | 2.39        | `[ref] osstream with filler string` |
| 28,366.48             | 35,252.88             | 0.7%      | 2.42        | `osstream << int (not C locale)`    |
| 1,045.82              | 956,183.67            | 0.7%      | 2.41        | `osstream << int (is C locale)`     |
| 9.08                  | 110,145,251.38        | 0.4%      | 2.42        | `std::to_string(int)`               |
| 4.65                  | 215,153,197.41        | 0.4%      | 2.40        | `std::to_chars(int)`                |
| 28,814.25             | 34,705.05             | 1.0%      | 2.42        | `osstream << double (not C locale)` |
| 1,368.22              | 730,877.02            | 1.4%      | 2.29        | `osstream << double (is C locale)`  |
| 348.63                | 2,868,353.50          | 1.8%      | 2.41        | `std::to_string(double)`            |
| 32.64                 | 30,638,029.72         | 0.4%      | 2.42        | `std::to_chars(double)`             |
|



With patch:
-----------

| ns/op                 | op/s                  | err%      | total       | benchmark                           |
| --------------------: | --------------------: | --------: | ----------: | :----------                         |
| 90.23                 | 11,083,182.99         | 0.9%      | 2.39        | `[ref] osstream with filler string` |
| 222.78                | 4,488,729.77          | 1.0%      | 2.43        | `osstream << int (not C locale)`    |
| 223.61                | 4,472,112.67          | 1.1%      | 2.42        | `osstream << int (is C locale)`     |
| 8.82                  | 113,315,748.74        | 1.6%      | 2.38        | `std::to_string(int)`               |
| 4.76                  | 209,994,184.81        | 2.0%      | 2.44        | `std::to_chars(int)`                |
| 482.47                | 2,072,672.21          | 1.4%      | 2.41        | `osstream << double (not C locale)` |
| 477.90                | 2,092,487.71          | 0.7%      | 2.42        | `osstream << double (is C locale)`  |
| 321.76                | 3,107,894.84          | 0.3%      | 2.42        | `std::to_string(double)`            |
| 32.55                 | 30,721,425.55         | 0.6%      | 2.46        | `std::to_chars(double)`             |


The previous code used __libcpp_locale_guard, which caused severe
performance issues with certain usage of streams because setlocale is
_that_ slow on Windows. This change speeds up number-to-string
conversion using ostringstream by a factor of 5x to 100x.

Fixes https://github.com/llvm/llvm-project/issues/56202


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128600

Files:
  libcxx/src/support/win32/locale_win32.cpp


Index: libcxx/src/support/win32/locale_win32.cpp
===================================================================
--- libcxx/src/support/win32/locale_win32.cpp
+++ libcxx/src/support/win32/locale_win32.cpp
@@ -96,11 +96,15 @@
         _CRT_INTERNAL_LOCAL_PRINTF_OPTIONS | _CRT_INTERNAL_PRINTF_STANDARD_SNPRINTF_BEHAVIOR,
         ret, n, format, loc, ap);
 #else
-    __libcpp_locale_guard __current(loc);
-    _LIBCPP_DIAGNOSTIC_PUSH
-    _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wformat-nonliteral")
-    int result = vsnprintf( ret, n, format, ap );
-    _LIBCPP_DIAGNOSTIC_POP
+    int result;
+    if (n != 0) {
+      va_list ap_copy;
+      va_copy(ap_copy, ap);
+      result = _vsnprintf_s_l(ret, n, _TRUNCATE, format, loc, ap_copy);
+      va_end(ap_copy);
+    }
+    if (n == 0 || result < 0)
+      result = _vscprintf_l(format, loc, ap);
 #endif
     va_end(ap);
     return result;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D128600.440041.patch
Type: text/x-patch
Size: 894 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220626/89569f6f/attachment-0001.bin>