Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Alexander Rukletsov]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/jsonify.hpp
Lines 123 (patched)


Can you please remove the trailing underscore then? It is weird to see it 
in a public symbol.



3rdparty/stout/include/stout/jsonify.hpp
Lines 366-368 (original), 273-275 (patched)


Do we still want / need a `CHECK` then?



3rdparty/stout/include/stout/jsonify.hpp
Line 375 (original), 282-284 (patched)


ditto


- Alexander Rukletsov


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:
> 
> minq1q3   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/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

> On July 20, 2018, 2:29 p.m., Michael Park wrote:
> > Looks good!
> > 
> > It might be worth considering using the `OStreamWrapper` stuff for the 
> > `ostream` API.
> > I know writing to `StringBuffer` is faster than writing to 
> > `OStreamWrapper`, but
> > I think just writing to `OStreamWrapper` would be faster than writing to 
> > `StringBuffer`
> > then copying that into a `std::string` then writing that to `ostream` in 
> > the end anyway.
> 
> Benjamin Mahler wrote:
> > I think just writing to OStreamWrapper would be faster than writing to 
> StringBuffer
> 
> That had seemed doubtful to me based on their documentation:
> http://rapidjson.org/md_doc_stream.html#iostreamWrapper
> 
> I would be interesting to get the numbers! However, we don't write to 
> ostream in the end any longer:
> https://reviews.apache.org/r/67992/
> 
> I could remove the `<<` operator but I figured I would just leave it 
> untouched for now.

Yeah, you left out the second half of my sentence but I assume it wasn't 
intentional.
I didn't notice the patch to replace `ostringstream`. Sounds good to me!


> On July 20, 2018, 2:29 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 137 (original), 95 (patched)
> > 
> >
> > Looks like `GetString` returns a `const char*`. We should provide the 
> > length here: `{buffer.GetString(), buffer.GetSize()}`.
> 
> Benjamin Mahler wrote:
> Ah nice catch! This should avoid walking the string right? I'll re-run 
> the numbers.

Yep, exactly.


- Michael


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


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.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:
> 
> minq1q3   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/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Benjamin Mahler]

> On July 20, 2018, 10:58 a.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 187 (original), 145 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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:
> 
> minq1q3   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
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Benjamin Mahler]

> On July 20, 2018, 9:29 p.m., Michael Park wrote:
> > Looks good!
> > 
> > It might be worth considering using the `OStreamWrapper` stuff for the 
> > `ostream` API.
> > I know writing to `StringBuffer` is faster than writing to 
> > `OStreamWrapper`, but
> > I think just writing to `OStreamWrapper` would be faster than writing to 
> > `StringBuffer`
> > then copying that into a `std::string` then writing that to `ostream` in 
> > the end anyway.

> I think just writing to OStreamWrapper would be faster than writing to 
> StringBuffer

That had seemed doubtful to me based on their documentation:
http://rapidjson.org/md_doc_stream.html#iostreamWrapper

I would be interesting to get the numbers! However, we don't write to ostream 
in the end any longer:
https://reviews.apache.org/r/67992/

I could remove the `<<` operator but I figured I would just leave it untouched 
for now.


> On July 20, 2018, 9:29 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 137 (original), 95 (patched)
> > 
> >
> > Looks like `GetString` returns a `const char*`. We should provide the 
> > length here: `{buffer.GetString(), buffer.GetSize()}`.

Ah nice catch! This should avoid walking the string right? I'll re-run the 
numbers.


- Benjamin


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


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

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

> On July 20, 2018, 3:58 a.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 370 (original), 278 (patched)
> > 
> >
> > If we care enough about performance to make a templatized overload for 
> > `const char(&)[N]`, maybe we should also add one for `std::string&&`?

The `const char (&)[N]` exists to avoid constructing a temporary
`std::string` from a string literal argument at the moment.
What cases would adding `std::string&&` improve? We can't exactly
move the `string` anyway right?


- Michael


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


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.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:
> 
> minq1q3   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
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

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


Fix it, then Ship it!




Looks good!

It might be worth considering using the `OStreamWrapper` stuff for the 
`ostream` API.
I know writing to `StringBuffer` is faster than writing to `OStreamWrapper`, but
I think just writing to `OStreamWrapper` would be faster than writing to 
`StringBuffer`
then copying that into a `std::string` then writing that to `ostream` in the 
end anyway.


3rdparty/stout/include/stout/jsonify.hpp
Line 137 (original), 95 (patched)


Looks like `GetString` returns a `const char*`. We should provide the 
length here: `{buffer.GetString(), buffer.GetSize()}`.


- Michael Park


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.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:
> 
> minq1q3   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
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Benno Evers]

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


Let's not use C-style casts.



3rdparty/stout/include/stout/jsonify.hpp
Line 187 (original), 145 (patched)


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)


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)


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*, 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)


This seems to be missing a word? (same below)



3rdparty/stout/include/stout/jsonify.hpp
Line 370 (original), 278 (patched)


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)


I'd suggest `empty_` for consistency.



3rdparty/stout/include/stout/jsonify.hpp
Line 417 (original), 324 (patched)


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

Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Benjamin Mahler]

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

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:

minq1q3   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