[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:356-357
+{
+    constexpr size_t size = 4 * sizeof(V) + 1; // +1 for null char from swprintf
+    typename S::value_type buf[size] = {};
+    const int len = sprintf_like(buf, size, fmt, a);
----------------
lebedev.ri wrote:
> `initial_string` should preallocate sufficiently-long string already.
> If it does not, it should be fixed, no?
> 
We would lost benefits from SSO if  `initial_string` preallocates sufficient long string.

Consider the following example:
```
const long long one = 1;
const auto s = to_wstring(one);
```

Implementation with preallocation does the following:

  - Allocate `4 * sizeof(long long) * sizeof(wchar_t)` bytes in the string (about 128 bytes)
  -  `swprintf` 1 into `initial_string`

I.e. 1 `malloc` call.

Proposed solution:

  - make buffer on stack of size `(4 * sizeof(long long) + 1) * sizeof(wchar_t)` (about 132 bytes) (no allocation)
  - `swprintf` 1 into buffer (i.e. 2 symbols)
  - construct `wstring` from 2 symbols of temporary buffer (<-- here is SSO used, no allocation)

I.e. 0 `malloc` calls


================
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);
+}
----------------
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.


================
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);
 }
----------------
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.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D59178