> On July 20, 2018, 10:58 a.m., Benno Evers wrote: > > 3rdparty/stout/include/stout/jsonify.hpp > > Line 187 (original), 145 (patched) > > <https://reviews.apache.org/r/67988/diff/1/?file=2061761#file2061761line216> > > > > 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.
`Bool()`, `Double()` and so on always return true, hence the check here. Alternatively I could ignore the returned boolean if that lowers the overhead for the reader (although hopefully they don't go off wondering why we ignore the returned boolean?). However, both `String()` and `Key()` can return flase only when write validation is turned on (per the comment on those). > On July 20, 2018, 10:58 a.m., Benno Evers wrote: > > 3rdparty/stout/include/stout/jsonify.hpp > > Line 417 (original), 324 (patched) > > <https://reviews.apache.org/r/67988/diff/1/?file=2061761#file2061761line458> > > > > 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 > > "{}". Hm.. I wanted to communicate the json structures that get written rather than the characters that go into the buffer, so I said things like "an empty object" / "empty array" / "empty string". Perhaps this would be clearer as "empty json object" / "empty json array" / "empty json string"? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67988/#review206267 ----------------------------------------------------------- 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 > >
