mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.
Date Fri, 20 Jul 2018 10:58:07 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message