----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67988/#review206267 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/stout/include/stout/jsonify.hpp Line 169 (original), 129 (patched) <https://reviews.apache.org/r/67988/#comment289163> Let's not use C-style casts. 3rdparty/stout/include/stout/jsonify.hpp Line 187 (original), 145 (patched) <https://reviews.apache.org/r/67988/#comment289165> I would suggest to add a comment explaining that rapidjson itself says that the intended way of handling a `false` return value is to terminate the process, otherwise this looks like a very extreme way of error handling. 3rdparty/stout/include/stout/jsonify.hpp Line 327 (original), 251 (patched) <https://reviews.apache.org/r/67988/#comment289174> The comment seems outdated, `set()` can only be used to write strings, not characters. Also, just a personal preference, but I would suggest referring to functions using `()` after the function name, i.e. `set()` instead of `set`. 3rdparty/stout/include/stout/jsonify.hpp Line 329 (original), 254 (patched) <https://reviews.apache.org/r/67988/#comment289175> I don't really have a good suggestion on how to change it, but this whole class seems a bit weird now - we instantiate an object whose only purpose it is to have one member function be called, exactly once, and it can't even check that because there is no way of signalling the error. Instinctively, this should probably just free functions like `void writeString(rapidjson::Writer<rapidjson::StringBuffer>*, const std::string&);`, but that doesn't really fit within the rest of stouts json framework. 3rdparty/stout/include/stout/jsonify.hpp Line 366 (original), 273 (patched) <https://reviews.apache.org/r/67988/#comment289166> This seems to be missing a word? (same below) 3rdparty/stout/include/stout/jsonify.hpp Line 370 (original), 278 (patched) <https://reviews.apache.org/r/67988/#comment289176> If we care enough about performance to make a templatized overload for `const char(&)[N]`, maybe we should also add one for `std::string&&`? 3rdparty/stout/include/stout/jsonify.hpp Lines 289 (patched) <https://reviews.apache.org/r/67988/#comment289173> I'd suggest `empty_` for consistency. 3rdparty/stout/include/stout/jsonify.hpp Line 417 (original), 324 (patched) <https://reviews.apache.org/r/67988/#comment289172> I feel like the previous comment was a little bit more helpful, since its not immediately obvious that an empty object corresponds to the string "{}". - Benno Evers On July 20, 2018, 3:38 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67988/ > ----------------------------------------------------------- > > (Updated July 20, 2018, 3:38 a.m.) > > > Review request for mesos, Alexander Rukletsov, Benno Evers, and Michael Park. > > > Bugs: MESOS-9092 > https://issues.apache.org/jira/browse/MESOS-9092 > > > Repository: mesos > > > Description > ------- > > This reduces the time needed for the client to finish receiving a > master's /state response by 50% in the `StateQuery` benchmark: > > min q1 q3 max > baseline 6.52 6.76 7.33 8.26 > rapidjson w/ SIMD 3.48 3.54 4.12 4.4 > rapidjson 3.29 3.32 3.65 3.85 > > SIMD is left disabled for now since it showed slightly slower > results. > > > Diffs > ----- > > 3rdparty/stout/include/stout/jsonify.hpp > 2314980e185ee61cc2ea54f1b4d2a8b35e58121c > > > Diff: https://reviews.apache.org/r/67988/diff/1/ > > > Testing > ------- > > Tested at the end of this chain, since this is split across > stout/libprocess/mesos. > > > Thanks, > > Benjamin Mahler > >
