Skip to content

[libc++] Refactor num_get optimization to not be ABI breaking #121690

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

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented Jan 5, 2025

----------------------------------------------------
Benchmark                             old        new
----------------------------------------------------
BM_num_get<bool>                  81.8 ns    79.6 ns
BM_num_get<long>                  80.2 ns    75.4 ns
BM_num_get<long long>             81.5 ns    76.4 ns
BM_num_get<unsigned short>        82.5 ns    78.4 ns
BM_num_get<unsigned int>          82.8 ns    78.6 ns
BM_num_get<unsigned long>         81.2 ns    78.1 ns
BM_num_get<unsigned long long>    83.6 ns    76.7 ns
BM_num_get<float>                  119 ns     120 ns
BM_num_get<double>                 113 ns     109 ns
BM_num_get<long double>            115 ns     119 ns
BM_num_get<void*>                  147 ns     139 ns

@philnik777 philnik777 marked this pull request as ready for review January 15, 2025 14:41
@philnik777 philnik777 requested a review from a team as a code owner January 15, 2025 14:41
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

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

3 Files Affected:

  • (modified) libcxx/include/__configuration/abi.h (-2)
  • (modified) libcxx/include/locale (+42-55)
  • (modified) libcxx/src/locale.cpp (+10)
diff --git a/libcxx/include/__configuration/abi.h b/libcxx/include/__configuration/abi.h
index c6ef6fdcdf96e6..5e40e308c8926c 100644
--- a/libcxx/include/__configuration/abi.h
+++ b/libcxx/include/__configuration/abi.h
@@ -57,8 +57,6 @@
 // because it changes the mangling of the virtual function located in the vtable, which
 // changes how it gets signed.
 #  define _LIBCPP_ABI_BAD_FUNCTION_CALL_GOOD_WHAT_MESSAGE
-// Enable optimized version of __do_get_(un)signed which avoids redundant copies.
-#  define _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
 // Give reverse_iterator<T> one data member of type T, not two.
 // Also, in C++17 and later, don't derive iterator types from std::iterator.
 #  define _LIBCPP_ABI_NO_ITERATOR_BASES
diff --git a/libcxx/include/locale b/libcxx/include/locale
index 981f25ed1e98cf..4fb01ce061c0b2 100644
--- a/libcxx/include/locale
+++ b/libcxx/include/locale
@@ -400,8 +400,9 @@ struct __num_get : protected __num_get_base {
       unsigned*& __g_end,
       unsigned& __dc,
       _CharT* __atoms);
-#    ifndef _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
-  static string __stage2_int_prep(ios_base& __iob, _CharT* __atoms, _CharT& __thousands_sep);
+
+  [[__deprecated__("This exists only for ABI compatability")]] static string
+  __stage2_int_prep(ios_base& __iob, _CharT* __atoms, _CharT& __thousands_sep);
   static int __stage2_int_loop(
       _CharT __ct,
       int __base,
@@ -414,55 +415,32 @@ struct __num_get : protected __num_get_base {
       unsigned*& __g_end,
       _CharT* __atoms);
 
-#    else
-  static string __stage2_int_prep(ios_base& __iob, _CharT& __thousands_sep) {
+  _LIBCPP_HIDE_FROM_ABI static string __stage2_int_prep(ios_base& __iob, _CharT& __thousands_sep) {
     locale __loc                 = __iob.getloc();
     const numpunct<_CharT>& __np = use_facet<numpunct<_CharT> >(__loc);
     __thousands_sep              = __np.thousands_sep();
     return __np.grouping();
   }
 
-  const _CharT* __do_widen(ios_base& __iob, _CharT* __atoms) const { return __do_widen_p(__iob, __atoms); }
-
-  static int __stage2_int_loop(
-      _CharT __ct,
-      int __base,
-      char* __a,
-      char*& __a_end,
-      unsigned& __dc,
-      _CharT __thousands_sep,
-      const string& __grouping,
-      unsigned* __g,
-      unsigned*& __g_end,
-      const _CharT* __atoms);
+  _LIBCPP_HIDE_FROM_ABI const _CharT* __do_widen(ios_base& __iob, _CharT* __atoms) const {
+    return __do_widen_p(__iob, __atoms);
+  }
 
 private:
   template <typename _Tp>
-  const _Tp* __do_widen_p(ios_base& __iob, _Tp* __atoms) const {
+  _LIBCPP_HIDE_FROM_ABI const _Tp* __do_widen_p(ios_base& __iob, _Tp* __atoms) const {
     locale __loc = __iob.getloc();
     use_facet<ctype<_Tp> >(__loc).widen(__src, __src + __int_chr_cnt, __atoms);
     return __atoms;
   }
 
-  const char* __do_widen_p(ios_base& __iob, char* __atoms) const {
+  _LIBCPP_HIDE_FROM_ABI const char* __do_widen_p(ios_base& __iob, char* __atoms) const {
     (void)__iob;
     (void)__atoms;
     return __src;
   }
-#    endif
 };
 
-#    ifndef _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
-template <class _CharT>
-string __num_get<_CharT>::__stage2_int_prep(ios_base& __iob, _CharT* __atoms, _CharT& __thousands_sep) {
-  locale __loc = __iob.getloc();
-  std::use_facet<ctype<_CharT> >(__loc).widen(__src, __src + __int_chr_cnt, __atoms);
-  const numpunct<_CharT>& __np = std::use_facet<numpunct<_CharT> >(__loc);
-  __thousands_sep              = __np.thousands_sep();
-  return __np.grouping();
-}
-#    endif
-
 template <class _CharT>
 string __num_get<_CharT>::__stage2_float_prep(
     ios_base& __iob, _CharT* __atoms, _CharT& __decimal_point, _CharT& __thousands_sep) {
@@ -475,18 +453,17 @@ string __num_get<_CharT>::__stage2_float_prep(
 }
 
 template <class _CharT>
-int
-#    ifndef _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
-__num_get<_CharT>::__stage2_int_loop(_CharT __ct, int __base, char* __a, char*& __a_end,
-                  unsigned& __dc, _CharT __thousands_sep, const string& __grouping,
-                  unsigned* __g, unsigned*& __g_end, _CharT* __atoms)
-#    else
-__num_get<_CharT>::__stage2_int_loop(_CharT __ct, int __base, char* __a, char*& __a_end,
-                  unsigned& __dc, _CharT __thousands_sep, const string& __grouping,
-                  unsigned* __g, unsigned*& __g_end, const _CharT* __atoms)
-
-#    endif
-{
+int __num_get<_CharT>::__stage2_int_loop(
+    _CharT __ct,
+    int __base,
+    char* __a,
+    char*& __a_end,
+    unsigned& __dc,
+    _CharT __thousands_sep,
+    const string& __grouping,
+    unsigned* __g,
+    unsigned*& __g_end,
+    _CharT* __atoms) {
   if (__a_end == __a && (__ct == __atoms[24] || __ct == __atoms[25])) {
     *__a_end++ = __ct == __atoms[24] ? '+' : '-';
     __dc       = 0;
@@ -856,14 +833,9 @@ _InputIterator num_get<_CharT, _InputIterator>::__do_get_signed(
   // Stage 2
   char_type __thousands_sep;
   const int __atoms_size = __num_get_base::__int_chr_cnt;
-#    ifdef _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
   char_type __atoms1[__atoms_size];
   const char_type* __atoms = this->__do_widen(__iob, __atoms1);
   string __grouping        = this->__stage2_int_prep(__iob, __thousands_sep);
-#    else
-  char_type __atoms[__atoms_size];
-  string __grouping = this->__stage2_int_prep(__iob, __atoms, __thousands_sep);
-#    endif
   string __buf;
   __buf.resize(__buf.capacity());
   char* __a     = &__buf[0];
@@ -879,7 +851,17 @@ _InputIterator num_get<_CharT, _InputIterator>::__do_get_signed(
       __a     = &__buf[0];
       __a_end = __a + __tmp;
     }
-    if (this->__stage2_int_loop(*__b, __base, __a, __a_end, __dc, __thousands_sep, __grouping, __g, __g_end, __atoms))
+    if (this->__stage2_int_loop(
+            *__b,
+            __base,
+            __a,
+            __a_end,
+            __dc,
+            __thousands_sep,
+            __grouping,
+            __g,
+            __g_end,
+            const_cast<char_type*>(__atoms)))
       break;
   }
   if (__grouping.size() != 0 && __g_end - __g < __num_get_base::__num_get_buf_sz)
@@ -905,14 +887,9 @@ _InputIterator num_get<_CharT, _InputIterator>::__do_get_unsigned(
   // Stage 2
   char_type __thousands_sep;
   const int __atoms_size = __num_get_base::__int_chr_cnt;
-#    ifdef _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
   char_type __atoms1[__atoms_size];
   const char_type* __atoms = this->__do_widen(__iob, __atoms1);
   string __grouping        = this->__stage2_int_prep(__iob, __thousands_sep);
-#    else
-  char_type __atoms[__atoms_size];
-  string __grouping = this->__stage2_int_prep(__iob, __atoms, __thousands_sep);
-#    endif
   string __buf;
   __buf.resize(__buf.capacity());
   char* __a     = &__buf[0];
@@ -928,7 +905,17 @@ _InputIterator num_get<_CharT, _InputIterator>::__do_get_unsigned(
       __a     = &__buf[0];
       __a_end = __a + __tmp;
     }
-    if (this->__stage2_int_loop(*__b, __base, __a, __a_end, __dc, __thousands_sep, __grouping, __g, __g_end, __atoms))
+    if (this->__stage2_int_loop(
+            *__b,
+            __base,
+            __a,
+            __a_end,
+            __dc,
+            __thousands_sep,
+            __grouping,
+            __g,
+            __g_end,
+            const_cast<char_type*>(__atoms)))
       break;
   }
   if (__grouping.size() != 0 && __g_end - __g < __num_get_base::__num_get_buf_sz)
diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp
index a1e10401f0b299..0b41136ea1e0a7 100644
--- a/libcxx/src/locale.cpp
+++ b/libcxx/src/locale.cpp
@@ -5645,6 +5645,16 @@ void moneypunct_byname<wchar_t, true>::init(const char* nm) {
 
 void __do_nothing(void*) {}
 
+// Legacy ABI __num_get functions - the new ones are _LIBCPP_HIDE_FROM_ABI
+template <class _CharT>
+string __num_get<_CharT>::__stage2_int_prep(ios_base& __iob, _CharT* __atoms, _CharT& __thousands_sep) {
+  locale __loc = __iob.getloc();
+  std::use_facet<ctype<_CharT> >(__loc).widen(__src, __src + __int_chr_cnt, __atoms);
+  const numpunct<_CharT>& __np = std::use_facet<numpunct<_CharT> >(__loc);
+  __thousands_sep              = __np.thousands_sep();
+  return __np.grouping();
+}
+
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS collate<char>;
 _LIBCPP_IF_WIDE_CHARACTERS(template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS collate<wchar_t>;)
 

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.

LGTM with comments applied.

@philnik777 philnik777 merged commit 956cfa6 into llvm:main Jan 30, 2025
11 of 15 checks passed
@philnik777 philnik777 deleted the apply_num_get_opt branch January 30, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants