Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Joris Van Remoortere]

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


Fix it, then Ship it!




+ resolution with neil's comments.


3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 152)


const?


- Joris Van Remoortere


On Jan. 31, 2016, 4:51 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43023/
> ---
> 
> (Updated Jan. 31, 2016, 4:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this patch + https://reviews.apache.org/r/43024/, the number of calls to 
> `operator new` and `operator delete` were reduced by roughly 1/3.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
> addec8ec6504e2a8f5b838fce3ebd4db224ab022 
> 
> Diff: https://reviews.apache.org/r/43023/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Michael Park]

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

(Updated Feb. 1, 2016, 5:14 a.m.)


Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Changes
---

Addressed Neil and Joris' comments.


Bugs: MESOS-4566
https://issues.apache.org/jira/browse/MESOS-4566


Repository: mesos


Description (updated)
---

With this + https://reviews.apache.org/r/43024/, the number of calls to
`operator new` and `operator delete` were reduced by roughly 1/3.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
f9d72243bcd5ae951ae1837fef0842915ff1e896 

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


Testing
---

`make check`


Thanks,

Michael Park

Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Michael Park]

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

Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Bugs: MESOS-4566
https://issues.apache.org/jira/browse/MESOS-4566


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
addec8ec6504e2a8f5b838fce3ebd4db224ab022 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Neil Conway]

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




3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 151)


Not yours, but I think initializing this buffer to `{}` is not good style. 
(It suggests there might be code paths in which the buffer is not subsequently 
assigned to. If we leave it uninitialized, we get a compiler warning if we try 
to read from it before setting it, which is actually what we want.)



3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 162)


I assume we have some evidence that this optimization is warranted?



3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 173)


This seems a little more subtle than is warranted.


- Neil Conway


On Jan. 31, 2016, 12:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43023/
> ---
> 
> (Updated Jan. 31, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
> addec8ec6504e2a8f5b838fce3ebd4db224ab022 
> 
> Diff: https://reviews.apache.org/r/43023/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Michael Park]

> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 162
> > 
> >
> > I assume we have some evidence that this optimization is warranted?

With this patch + https://reviews.apache.org/r/43024/, the # of calls to 
`operator new` and `operator delete` reduces by roughly 1/3.


- Michael


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


On Jan. 31, 2016, 12:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43023/
> ---
> 
> (Updated Jan. 31, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
> addec8ec6504e2a8f5b838fce3ebd4db224ab022 
> 
> Diff: https://reviews.apache.org/r/43023/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Michael Park]

> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 173
> > 
> >
> > This seems a little more subtle than is warranted.

The `i` part perhaps? Is it better if we were to call it `back`?
```
*stream_ << buffer << (buffer[back] == '.' ? "0" : "");
```


- Michael


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


On Jan. 31, 2016, 12:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43023/
> ---
> 
> (Updated Jan. 31, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
> addec8ec6504e2a8f5b838fce3ebd4db224ab022 
> 
> Diff: https://reviews.apache.org/r/43023/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Michael Park]

> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 162
> > 
> >
> > I assume we have some evidence that this optimization is warranted?
> 
> Michael Park wrote:
> With this patch + https://reviews.apache.org/r/43024/, the # of calls to 
> `operator new` and `operator delete` reduces by roughly 1/3.
> 
> Neil Conway wrote:
> Can we add this to the commit message?

Yep, added to the description for now.


> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 173
> > 
> >
> > This seems a little more subtle than is warranted.
> 
> Michael Park wrote:
> The `i` part perhaps? Is it better if we were to call it `back`?
> ```
> *stream_ << buffer << (buffer[back] == '.' ? "0" : "");
> ```
> 
> Neil Conway wrote:
> Yeah -- not clear that "i" never points to the NUL character for 
> non-empty strings, etc. Probably would be clearer without the trinary 
> expression.

The ternary expression was there from before though, that's not something to be 
"warranted" right?
Are you ok with this if I were to `s/i/back/`? or is it not sufficient?


- Michael


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


On Jan. 31, 2016, 12:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43023/
> ---
> 
> (Updated Jan. 31, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
> addec8ec6504e2a8f5b838fce3ebd4db224ab022 
> 
> Diff: https://reviews.apache.org/r/43023/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Michael Park]

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

(Updated Jan. 31, 2016, 4:51 a.m.)


Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


Bugs: MESOS-4566
https://issues.apache.org/jira/browse/MESOS-4566


Repository: mesos


Description (updated)
---

With this patch + https://reviews.apache.org/r/43024/, the number of calls to 
`operator new` and `operator delete` were reduced by roughly 1/3.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
addec8ec6504e2a8f5b838fce3ebd4db224ab022 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 43023: Avoid construction of temporary strings in `NumberWriter` for doubles.

[Neil Conway]

> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 173
> > 
> >
> > This seems a little more subtle than is warranted.
> 
> Michael Park wrote:
> The `i` part perhaps? Is it better if we were to call it `back`?
> ```
> *stream_ << buffer << (buffer[back] == '.' ? "0" : "");
> ```

Yeah -- not clear that "i" never points to the NUL character for non-empty 
strings, etc. Probably would be clearer without the trinary expression.


> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 162
> > 
> >
> > I assume we have some evidence that this optimization is warranted?
> 
> Michael Park wrote:
> With this patch + https://reviews.apache.org/r/43024/, the # of calls to 
> `operator new` and `operator delete` reduces by roughly 1/3.

Can we add this to the commit message?


- Neil


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


On Jan. 31, 2016, 12:06 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43023/
> ---
> 
> (Updated Jan. 31, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
> addec8ec6504e2a8f5b838fce3ebd4db224ab022 
> 
> Diff: https://reviews.apache.org/r/43023/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>