> On Oct. 15, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > I have a feeling that if you kept around a stringstream with the local set 
> > your benchmarks would look rather different.
> > I also suggest using callgrind to get the instruction count / # of library 
> > calls made.

That occurred to me, but then some issues appeared with the idea. The version 
in `json.hpp` uses a free function, the way to keep the `stringstream` is by 
making it global (either having a global variable or marking it `static`), 
however that makes the function non thread safe and a mutex will be needed. It 
probably will still perform better in a single thread situation though.

The `jsonify.hpp` version has more or less the same concerns. I though moving 
the `stringstream` to the constructor of the `Number` object, which would fix 
the thread safety issue, but at the end one is just moving the object 
construction penalty from serialization to construction time. But given the 
usage of `jsonify`, it wouldn't have any performance issue.

Do you have any other idea on how to keep the `stringstream` alive?


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52877/#review152761
-----------------------------------------------------------


On Oct. 14, 2016, 2:50 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52877/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2016, 2:50 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Michael Park.
> 
> 
> Bugs: MESOS-6349
>     https://issues.apache.org/jira/browse/MESOS-6349
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Resolves an isse with the JSON serialization functions where floats
> would use the process active locale as a decimal separator instead
> of the JSON conformant period (`.`).
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/json.hpp 
> 9734561549eea867d57f14a0ab71febeb9dddc06 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 8220d001001e8b9f247c05be58534f1174611aad 
> 
> Diff: https://reviews.apache.org/r/52877/diff/
> 
> 
> Testing
> -------
> 
> Three types of tests were used for this issue. The first just run our current
> test suite (`make check`). The second verifies that the proposed solutions
> are able to generate a locale independent output and the third consist of a
> benchmark of all the solutions.
> 
> The verification that the proposed solutions produce locale independent JSON,
> the following program was used (with manual verification of the generated
> output):
> 
> ```c++
> #include <stout/json.hpp>
> 
> #include <clocale>
> #include <iostream>
> #include <locale>
> 
> int main(int argc, char **argv) {
>   // Set locale to German.
>   std::setlocale(LC_ALL, "de_DE.UTF-8");
>   std::cout.imbue(std::locale("de_DE.UTF-8"));
> 
>   JSON::Object object;
>   object.values["float"] = 1234567890.12345;
> 
>   std::cout << stringify(object) << '\n';
>   return 0;
> }
> 
> ```
> 
> In order to test the different options the following benchmarking program was
> written using [Google benchmark](https://github.com/google/benchmark):
> 
> ```c++
> #include <benchmark/benchmark_api.h>
> #include <stout/json.hpp>
> 
> #include <iostream>
> #include <random>
> #include <string>
> #include <sstream>
> 
> static void BM_Jsonify(benchmark::State& state) {
>   while (state.KeepRunning()) {
>     state.PauseTiming();
>     JSON::Object object;
>     object.values["float00"] = 1234567890.12345;
>     object.values["float01"] = 123456789.012345;
>     object.values["float02"] = 12345678.9012345;
>     object.values["float03"] = 1234567.89012345;
>     object.values["float04"] = 123456.789012345;
>     object.values["float05"] = 12345.6789012345;
>     object.values["float06"] = 1234.56789012345;
>     object.values["float07"] = 123.456789012345;
>     object.values["float08"] = 12.3456789012345;
>     object.values["float09"] = 1.23456789012345;
>     object.values["float10"] = 1234567890.12345;
>     object.values["float11"] = 123456789.012345;
>     object.values["float12"] = 12345678.9012345;
>     object.values["float13"] = 1234567.89012345;
>     object.values["float14"] = 123456.789012345;
>     object.values["float15"] = 12345.6789012345;
>     object.values["float16"] = 1234.56789012345;
>     object.values["float17"] = 123.456789012345;
>     object.values["float18"] = 12.3456789012345;
>     object.values["float19"] = 1.23456789012345;
>     object.values["float20"] = 1234567890.12345;
>     object.values["float21"] = 123456789.012345;
>     object.values["float22"] = 12345678.9012345;
>     object.values["float23"] = 1234567.89012345;
>     object.values["float24"] = 123456.789012345;
>     object.values["float25"] = 12345.6789012345;
>     object.values["float26"] = 1234.56789012345;
>     object.values["float27"] = 123.456789012345;
>     object.values["float28"] = 12.3456789012345;
>     object.values["float29"] = 1.23456789012345;
>     object.values["float30"] = 1234567890.12345;
>     object.values["float31"] = 123456789.012345;
>     object.values["float32"] = 12345678.9012345;
>     object.values["float33"] = 1234567.89012345;
>     object.values["float34"] = 123456.789012345;
>     object.values["float35"] = 12345.6789012345;
>     object.values["float36"] = 1234.56789012345;
>     object.values["float37"] = 123.456789012345;
>     object.values["float38"] = 12.3456789012345;
>     object.values["float39"] = 1.23456789012345;
>     object.values["float40"] = 1234567890.12345;
>     object.values["float41"] = 123456789.012345;
>     object.values["float42"] = 12345678.9012345;
>     object.values["float43"] = 1234567.89012345;
>     object.values["float44"] = 123456.789012345;
>     object.values["float45"] = 12345.6789012345;
>     object.values["float46"] = 1234.56789012345;
>     object.values["float47"] = 123.456789012345;
>     object.values["float48"] = 12.3456789012345;
>     object.values["float49"] = 1.23456789012345;
> 
>     state.ResumeTiming();
> 
>     benchmark::DoNotOptimize(stringify(object));
>   }
> }
> 
> BENCHMARK(BM_Jsonify);
> 
> BENCHMARK_MAIN();
> ```
> 
> The program creates a JSON object with 50 floats in it, and then it creates
> its string representation. Since the algorithms in `json.hpp` and 
> `jsonify.hpp`
> are similar but not the same, they were benchmarked as different. While the
> original solutions are non solutions since they produce erroneous results if
> localization is active, they constitute a baseline for comparison with the
> alternatives. These are the benchmarks for the original algorithms and their
> alternatives:
> 
> 1. `json.hpp` default:
> 
> ```c++
> char buffer[50] {};
> snprintf(
>     buffer,
>     sizeof(buffer),
>     "%#.*g",
>     std::numeric_limits<double>::digits10,
>     number.value);
> 
> std::string trimmed = strings::trim(buffer, strings::SUFFIX, "0");
> return out << trimmed << (trimmed.back() == '.' ? "0" : "");
> ```
> 
>    Perfomance
> 
> ```
> Run on (8 X 1000 MHz CPU s)
> 2016-10-14 13:25:01
> Benchmark    Time(ns)    CPU(ns) Iterations
> -------------------------------------------
> BM_Jsonify      67878      67597      10516
> ```
> 
> 2. `jsonify.hpp` default (it changes the call to `strings::strim` in order to
>    avoid string copies:
> 
> ```c++
> char buffer[50] {};
> const int size = snprintf(
>     buffer,
>     sizeof(buffer),
>     "%#.*g",
>     std::numeric_limits<double>::digits10,
>     number.value);
> 
> int back = size - 1;
> for (; back > 0; --back) {
>   if (buffer[back] != '0') {
>     break;
>   }
>   buffer[back] = '\0';
> }
> return out << buffer << (buffer[back] == '.' ? "0" : "");
> ```
> 
>    Performance
> 
> ```
> Run on (8 X 1000 MHz CPU s)
> 2016-10-14 13:27:32
> Benchmark    Time(ns)    CPU(ns) Iterations
> -------------------------------------------
> BM_Jsonify      65511      65204      10793
> ```
> 
> 3. Proposal 1, manually searching for a character which is not expected (and 
> it
>    is assumed to be the decimal separator) and replacing it:
> 
> ```c++
> char buffer[50] {};
> snprintf(
>     buffer,
>     sizeof(buffer),
>     "%#.*g",
>     std::numeric_limits<double>::digits10,
>     number.value);
> 
> for (char &ch : buffer) {
>   if (!isdigit(ch) && ch != '-' && ch != '.' && ch!= '\0') {
>     ch = '.';
>     break;
>   }
> }
> 
> std::string trimmed = strings::trim(buffer, strings::SUFFIX, "0");
> return out << trimmed << (trimmed.back() == '.' ? "0" : "");
> ```
> 
>    Performance
> 
> ```
> Run on (8 X 1000 MHz CPU s)
> 2016-10-14 13:32:04
> Benchmark    Time(ns)    CPU(ns) Iterations
> -------------------------------------------
> BM_Jsonify      69483      69201       9754
> ```
> 
> 4. Proposal 2, the C++ approach, use streams and stream manipulators:
> 
> ```c++
> std::stringstream ss;
> ss.imbue(std::locale::classic());
> ss << std::setprecision(std::numeric_limits<double>::digits10)
>    << std::showpoint << number.value;
> 
> std::string trimmed = strings::trim(ss.str(), strings::SUFFIX, "0");
> return out << trimmed << (trimmed.back() == '.' ? "0" : "");
> ```
> 
>    Performance
> 
> ```
> Run on (8 X 1000 MHz CPU s)
> 2016-10-14 13:33:22
> Benchmark    Time(ns)    CPU(ns) Iterations
> -------------------------------------------
> BM_Jsonify      82363      82337       8591
> ```
> 
> 5. Proposal 3, try to avoid copies (there are still some being created):
> 
> ```c++
> std::stringstream ss;
> ss.imbue(std::locale::classic());
> ss << std::setprecision(std::numeric_limits<double>::digits10)
>    << std::showpoint << number.value;
> 
> std::string representation = ss.str();
> int back = representation.size() - 1;
> for (; back > 0; --back) {
>   if (representation[back] != '0') {
>     break;
>   }
> }
> representation.resize(back + 1);
> out << representation << (representation[back] == '.' ? "0" : "");
> ```
> 
>    Performance
> 
> ```
> Run on (8 X 1000 MHz CPU s)
> 2016-10-14 13:34:40
> Benchmark    Time(ns)    CPU(ns) Iterations
> -------------------------------------------
> BM_Jsonify      94267      94235       7166
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to