You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2017/01/03 14:23:26 UTC

Re: Review Request 52877: Fixed wrong float serialization in JSON in locales different from C.

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

(Updated Jan. 3, 2017, 3:23 p.m.)


Review request for mesos, Adam B, Benjamin Bannier, Joris Van Remoortere, and Michael Park.


Changes
-------

Rebases master.
Rewrites the tests as follows:
- Test with `jsonify` instead of `stringify` since usage of `JSON::Object` adds unnecesary time noise.
- Tests with a random set of strings to avoid compiler optimizations (the same doubles were being repeated over and over again).
- Updated proposals to their latest versions.


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 (updated)
-----

  3rdparty/stout/include/stout/json.hpp 62ce15274677112d142a3c829b4a9f06258c9e2c 
  3rdparty/stout/include/stout/jsonify.hpp 8220d001001e8b9f247c05be58534f1174611aad 

Diff: https://reviews.apache.org/r/52877/diff/


Testing (updated)
-------

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();

    // Randomly generated set of numbers.
    std::vector<double> doubles = {
      54366462691.1798,
      3.35465250645312,
      3056184950.05953,
      74057564.7741182,
      280.445791893924,
      3446.63176368415,
      419012114.325581,
      3464212.51004162,
      1.45156507568354,
      13304750.4216248,
      7716457488648.00,
      700533630.650588,
      679.659801950981,
      307059152.688268,
      5615744.28063731,
      859.902033900705,
      293878.810967451,
      284685668.155903,
      722683811.462448,
      407.682284299325,
      9874834.88341080,
      4829649.14505646,
      3045544.72401146,
      1112707.08627010,
      8379539.79388719,
      3004161.89676627,
      3866662.79849617,
      3871151.73991937,
      4090119.26657417,
      4118699.88616345,
      2104416.18322520,
      9896898.63226234,
      5957851.08773457,
      3501068.52003430,
      7524714.36218293,
      4333874.01982647,
      9562008.06930384,
      3374957.45494027,
      5867075.07463260,
      815499.697450741,
      600936.470830026,
      9661425.72632153,
      6392256.71537575,
      7517969.33139398,
      9031912.03425553,
      5497593.85752735,
      815419.808032205,
      5098659.46057626,
      930160.667551563,
      5970944.98217500,
      6166534.92677787,
      3541537.67676275,
      1291933.13549156,
      9185094.72404290,
      4507338.03523123,
      9559754.89147696,
      6152898.39204769,
      2358966.41795446,
      6520510.92218183,
      2201757.78606032,
      4960487.80737309,
      1784969.91832029,
      3858390.23735070,
      1439952.27402359,
      6407199.91163080,
      6130379.95590661,
      6427890.23913244,
      2128879.29010338,
      8175291.42483598,
      1587278.27639235,
      7493343.68705307,
      4853439.25371342,
      2564845.15639735,
      1415661.63929173,
      6388168.79342734,
      3003424.90116775,
      6915390.10600792,
      7115390.65502377,
      5288515.90088063,
      1209208.86882085,
      4483111.91884606,
      5974377.97163572,
      5821054.89489766,
      8284136.84073623,
      1607044.34898051,
      3255087.31267763,
      2305369.43079747,
      1282392.57487249,
      4884797.49134467,
      5119420.62129117,
      6276725.07755672,
      3054999.92210194,
      3594116.58894982,
      6691568.49651968,
      265562.721872220,
      2864878.07276221,
      627299.902077148,
      5885179.44212665,
      7654144.98508934,
      590857.599685431
    };

    state.ResumeTiming();

    benchmark::DoNotOptimize(jsonify(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");
*stream_ << trimmed << (trimmed.back() == '.' ? "0" : "");
```

   Perfomance

```
Run on (8 X 2800 MHz CPU s)
2017-01-03 15:06:35
Benchmark           Time           CPU Iterations
-------------------------------------------------
BM_Jsonify        985 ns        986 ns     714227
```

2. `jsonify.hpp` default (it changes the call to `strings::trim` 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';
}
*stream_ << buffer << (buffer[back] == '.' ? "0" : "");
```

   Performance

```
Run on (8 X 2800 MHz CPU s)
2017-01-03 15:05:34
Benchmark           Time           CPU Iterations
-------------------------------------------------
BM_Jsonify        968 ns        969 ns     723417
```

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] {};
int size = snprintf(
    buffer,
    sizeof(buffer),
    "%#.*g",
    std::numeric_limits<double>::digits10,
    number.value);

static const char separator =
  std::use_facet<std::numpunct<char>>(stream.getloc()).decimal_point();
for (int i = 0; i < size; ++i) {
  if (buffer[i] == separator) {
    buffer[i] = '.';
    break;
  }
}

int back = size - 1;
for (; back > 0; --back) {
  if (buffer[back] != '0') {
    break;
  }
  buffer[back] = '\0';
}

*stream_ << buffer << (buffer[back] == '.' ? "0" : "");
```

   Performance

```
Run on (8 X 2800 MHz CPU s)
2017-01-03 15:07:39
Benchmark           Time           CPU Iterations
-------------------------------------------------
BM_Jsonify        982 ns        981 ns     704544
```

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 2800 MHz CPU s)
2017-01-03 15:09:40
Benchmark           Time           CPU Iterations
-------------------------------------------------
BM_Jsonify        977 ns        978 ns     715571
```

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);
*stream_ << representation << (representation[back] == '.' ? "0" : "");
```

   Performance

```
Run on (8 X 2800 MHz CPU s)
2017-01-03 15:10:52
Benchmark           Time           CPU Iterations
-------------------------------------------------
BM_Jsonify        982 ns        984 ns     714935
```

6. Proposal 4: Using thread local variables:

```c++
static THREAD_LOCAL std::stringstream* ss = nullptr;
static THREAD_LOCAL bool initialized = false;
if (!initialized) {
  ss = new std::stringstream;
  ss->imbue(std::locale::classic());
  *ss << std::setprecission(std::numeric_limits<double>::digits10)
      << std::showpoint;
  initialized = true;
}
*ss << double_;
std::string trimmed = strings::trim(ss->str(), strings::SUFFIX, "0");
*stream_ << trimmed << (trimmed.back() == '.' ? "0" : "");
ss->str("");
```

   Performance

```
Run on (8 X 2800 MHz CPU s)
2017-01-03 15:12:37
Benchmark           Time           CPU Iterations
-------------------------------------------------
BM_Jsonify        980 ns        980 ns     716413
```

Surprisingly, proposal 2 seemed to be the most efficient after multiple runs of the suite.


Thanks,

Alexander Rojas