Avoid copy of __atoms when char_type is char

[hiraditya]

Summary

The function num_get<_CharT>::stage2_int_prep makes unnecessary copy of src into atoms when char_type is char. This can be avoided by creating
a switch on type and just returning __src when char_type is char. Running a synthetic benchmark shows the impact of this change:

The test case can be found here: https://github.com/hiraditya/std-benchmark/blob/master/cxx/stringstream.bench.cpp

Without the change with llvm-project/trunk

$ export LD_LIBRARY_PATH=/work/llvm-project/install/lib; ./cxx/stringstream.bench.cpp.out
Run on (24 X 1200 MHz CPU s)
2017-02-22 14:37:34
Benchmark                        Time           CPU Iterations
--------------------------------------------------------------
BM_Istream_numbers/32         8328 ns       8336 ns      83121
BM_Istream_numbers/64         8312 ns       8320 ns      83754
BM_Istream_numbers/128        8301 ns       8309 ns      83975
BM_Istream_numbers/256        8298 ns       8306 ns      84349
BM_Istream_numbers/512        8303 ns       8311 ns      84308
BM_Istream_numbers/1024       8301 ns       8309 ns      84316

With the change on llvm-project/trunk

$ export LD_LIBRARY_PATH=/work/llvm-project/install-sstream/lib; ./cxx/stringstream.bench.cpp.out
Run on (24 X 1200 MHz CPU s)
2017-02-22 14:37:55
Benchmark                        Time           CPU Iterations
--------------------------------------------------------------
BM_Istream_numbers/32         7465 ns       7472 ns      91957
BM_Istream_numbers/64         7460 ns       7467 ns      93824
BM_Istream_numbers/128        7457 ns       7464 ns      93875
BM_Istream_numbers/256        7456 ns       7463 ns      93781
BM_Istream_numbers/512        7455 ns       7462 ns      93793
BM_Istream_numbers/1024       7457 ns       7464 ns      93757

[hiraditya]

Ping

[EricWF]

__num_get is externally instantiated. Adding non-template methods or changing method signatures is ABI breaking (As shown by the [check-cxx-abilist rule output](https://gist.github.com/EricWF/aeb561d6cd845128b828c41c42de09e8)).

This change needs to be made in a way that maintains ABI compatibility, or it needs to be guarded under a _LIBCPP_ABI_FOO macro and only exposed in ABI v2.

[hiraditya]

Guarded the new implementation with _LIBCPP_ABI_OPTIMIZED_LOCALE macro to avoid ABI incompatibilities.

[hiraditya]

ping

[hiraditya]

Ping

[EricWF]

OK, so ABI wise this seems good. I'll need a fresh head to review the locale changes for correctness. I'll get around to that tomorrow.

libcxx/include/locale

891 ↗

(On Diff #90214)

Would it be possible to reduce the amount of duplicate code between the two implementations? Currently it seems like there is a lot.

[hiraditya]

Minimized the diff by putting #ifdefs inside the function.

libcxx/include/locale

891 ↗

(On Diff #90214)

Sorry for the late response, I have tried to minimize the duplication.

[hiraditya]

Ping

[hiraditya]

Ping

[hiraditya]

Ping

[hiraditya]

Ping

[EricWF]

LGTM.

Could you please add the benchmark under libcxx/benchmarks.

Could you also make sure that the existing tests (under test/std) for number parsing have sufficient test coverages for the possible formats it might need to parse? Just to ensure this change isn't missing anything (or the old change isn't)

Once again sorry for the massive delay. <locale> patches are tricky, and I barely understand <locale> as is.

libcxx/include/__config

79 ↗

(On Diff #96159)

Could you make the macro name slightly more descriptive, in case we have further optimizations to locale :-)

Also please add a short comment describing the change.

libcxx/include/locale

969 ↗

(On Diff #96159)

Could you lift the 26 into a const int __atoms_size = 42 that's shared between both #if branches?

[hiraditya]

Addressed comments from Eric.

libcxx/trunk/benchmarks/stringstream.bench.cpp

+#include "benchmark/benchmark_api.h"
+
+#include <sstream>
+double __attribute__((noinline)) istream_numbers();
+
+double istream_numbers() {
+  const char *a[] = {
+    "-6  69 -71  2.4882e-02 -100 101 -2.00005 5000000 -50000000",
+    "-25 71   7 -9.3262e+01 -100 101 -2.00005 5000000 -50000000",
+    "-14 53  46 -6.7026e-02 -100 101 -2.00005 5000000 -50000000"
+  };
+
+  int a1, a2, a3, a4, a5, a6, a7;
+  double f1 = 0.0, f2 = 0.0, q = 0.0;
+  for (int i=0; i < 3; i++) {
+    std::istringstream s(a[i]);
+    s >> a1
+      >> a2
+      >> a3
+      >> f1
+      >> a4
+      >> a5
+      >> f2
+      >> a6
+      >> a7;
+    q += (a1 + a2 + a3 + a4 + a5 + a6 + a7 + f1 + f2)/1000000;
+  }
+  return q;
+}
+
+static void BM_Istream_numbers(benchmark::State &state) {
+  double i = 0;
+  while (state.KeepRunning())
+    benchmark::DoNotOptimize(i += istream_numbers());
+}
+
+BENCHMARK(BM_Istream_numbers)->RangeMultiplier(2)->Range(1024, 4096);
+BENCHMARK_MAIN()

libcxx/trunk/include/__config

 // instead of as a class simulating an enum. If this option is enabled
 // `pointer_safety` and `get_pointer_safety()` will no longer be available
 // in C++03.
 #define _LIBCPP_ABI_POINTER_SAFETY_ENUM_TYPE
 // Define a key function for `bad_function_call` in the library, to centralize
 // its vtable and typeinfo to libc++ rather than having all other libraries
 // using that class define their own copies.
 #define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
+
+// Enable optimized version of __do_get_(un)signed which avoids redundant copies.
+#define _LIBCPP_ABI_OPTIMIZED_LOCALE_NUM_GET
 #elif _LIBCPP_ABI_VERSION == 1
 #if !defined(_LIBCPP_OBJECT_FORMAT_COFF)
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries. This is unnecessary with
 // COFF dllexport semantics, since dllexport forces a non-inline definition
 // of inline functions to be emitted anyway. Our own non-inline copy would
 // conflict with the dllexport-emitted copy, so we disable it.
 #define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS

libcxx/trunk/include/locale

 _LIBCPP_FUNC_VIS
 void __check_grouping(const string& __grouping, unsigned* __g, unsigned* __g_end,
                       ios_base::iostate& __err);
 
 template <class _CharT>
 struct __num_get
     : protected __num_get_base
 {
-    static string __stage2_int_prep(ios_base& __iob, _CharT* __atoms, _CharT& __thousands_sep);
     static string __stage2_float_prep(ios_base& __iob, _CharT* __atoms, _CharT& __decimal_point,
                                       _CharT& __thousands_sep);
-    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, _CharT* __atoms);
+
     static int __stage2_float_loop(_CharT __ct, bool& __in_units, char& __exp,
                                    char* __a, char*& __a_end,
                                    _CharT __decimal_point, _CharT __thousands_sep,
                                    const string& __grouping, unsigned* __g,
                                    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);
+    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, _CharT* __atoms);
+
+#else
+    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);
+private:
+    template<typename T>
+    const T* __do_widen_p(ios_base& __iob, T* __atoms) const
+    {
+      locale __loc = __iob.getloc();
+      use_facet<ctype<T> >(__loc).widen(__src, __src + 26, __atoms);
+      return __atoms;
+    }
+
+    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();
     use_facet<ctype<_CharT> >(__loc).widen(__src, __src + 26, __atoms);
     const numpunct<_CharT>& __np = 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)
 {
     locale __loc = __iob.getloc();
     use_facet<ctype<_CharT> >(__loc).widen(__src, __src + 32, __atoms);
     const numpunct<_CharT>& __np = use_facet<numpunct<_CharT> >(__loc);
     __decimal_point = __np.decimal_point();
     __thousands_sep = __np.thousands_sep();
     return __np.grouping();
 }
 
 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
 {
     if (__a_end == __a && (__ct == __atoms[24] || __ct == __atoms[25]))
     {
         *__a_end++ = __ct == __atoms[24] ? '+' : '-';
         __dc = 0;
         return 0;
     }
     if (__grouping.size() != 0 && __ct == __thousands_sep)

 num_get<_CharT, _InputIterator>::__do_get_signed(iter_type __b, iter_type __e,
                                         ios_base& __iob,
                                         ios_base::iostate& __err,
                                         _Signed& __v) const
 {
     // Stage 1
     int __base = this->__get_base(__iob);
     // Stage 2
-    char_type __atoms[26];
     char_type __thousands_sep;
+    const int __atoms_size = 26;
+#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];
     char* __a_end = __a;
     unsigned __g[__num_get_base::__num_get_buf_sz];
     unsigned* __g_end = __g;
     unsigned __dc = 0;
     for (; __b != __e; ++__b)

 num_get<_CharT, _InputIterator>::__do_get_unsigned(iter_type __b, iter_type __e,
                                         ios_base& __iob,
                                         ios_base::iostate& __err,
                                         _Unsigned& __v) const
 {
     // Stage 1
     int __base = this->__get_base(__iob);
     // Stage 2
-    char_type __atoms[26];
     char_type __thousands_sep;
+    const int __atoms_size = 26;
+#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];
     char* __a_end = __a;
     unsigned __g[__num_get_base::__num_get_buf_sz];
     unsigned* __g_end = __g;
     unsigned __dc = 0;
     for (; __b != __e; ++__b)