[libcxx-commits] [PATCH] D59178: [libc++] Speedup to_string and to_wstring for integers using stack buffer and SSO

[Afanasyev Ivan via Phabricator via libcxx-commits]

ivafanas added inline comments.


================
Comment at: libcxx/src/string.cpp:358-359
+    typename S::value_type buf[size] = {};
+    const int len = sprintf_like(buf, size, fmt, a);
+    return S(buf, buf + len);
+}
----------------
mclow.lists wrote:
> ivafanas wrote:
> > lebedev.ri wrote:
> > > Missing error checking.
> > > ```
> > > RETURN VALUE
> > >        Upon successful return, these functions return the number of characters printed (excluding the null byte used to end output to strings).
> > > 
> > > ```
> > `size` is chosen in the way that there should be enough space for integer:
> > `4 * sizeof(V)`: 2^8 = 256 - 3 digits + 1 digit for minus.
> > 
> > GCC implementation ignores negative return values so as they knows that space is enough:
> > https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/ext/string_conversions.h#L99
> > 
> > In case of unexpected `sprintf` failure I will add fallback to the previous algorithm.
> Because of licensing issues, we aren't able to refer to GCC's source code.
> Please help us do so by not posting links, snippets, etc from GCC here.
> 
Ooooops, sorry, didn't know about that.

Also I didnn't found how to edit / delete comment with link to gcc repo. If there is a way, please help me.


================
Comment at: libcxx/src/string.cpp:422
 {
-    return as_string(snprintf, initial_string<string, int>()(), "%d", val);
+    return i_to_string<string>(snprintf, "%d", val);
 }
----------------
ivafanas wrote:
> ed wrote:
> > Considering that performance is of the essence and that we only do simple conversions (without leading whitespace, zeroes, etc.), would it make sense to handroll this for the integer cases? This is likely a lot faster than using `snprintf()`.
> I will try to implement that and estimate performance gain.
Ed, I have locally implemented simple conversion and it gives significant performance boost (about 20x times when SSO plays).

There is one more question I'm looking for answer for days.

According to `std::to_string` documentation result for integer types must be equal to `sprintf` call with `%i` parameter (`%l` for long etc etc)
https://en.cppreference.com/w/cpp/string/basic_string/to_string

According to `to_chars` documentation `to_chars` is the only locale-independent formatter
https://en.cppreference.com/w/cpp/utility/to_chars

According to this note clang aims to work with the system C library
http://clang-developers.42468.n3.nabble.com/C-Standard-Library-td2127533.html

I've checked `glibc`, `ditelibc` and `musl` standard C lib implementations, and they provides the expected result for `sprintf(s, "%i", x)` calls without locale checks for thousands separator: `sprintf(s, "%i", 100500) --> 100500`
I've tested msvc `sprintf(s, "%i", x)` calls with the different locales, it also prints integers without thousands separator.

So, on the one side, performance boost is really impressive, on the other side there is no guarantee that `sprintf(s, "%i", x)` will always ignore thousands separator.

I want to ask your opinion as much more experienced compiler developer.
Should we use fast simple conversion or leave slow `sprintf` calls?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59178/new/

https://reviews.llvm.org/D59178