> On Oct. 14, 2016, 11:23 p.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. > > Alexander Rojas wrote: > 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? > > Joris Van Remoortere wrote: > Thread Local? > > Alexander Rojas wrote: > From the `stout/thread_local.hpp` > > ```c++ > // We required that THREAD_LOCAL is only used with POD types as this > // is a requirement of `__thread`. > ``` > > `std::stringstream` is definitely not a POD.
`std::stringstream*` is - Joris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52877/#review152761 ----------------------------------------------------------- On Oct. 14, 2016, 12: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, 12: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 > >
