You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2016/01/31 01:06:12 UTC
Review Request 43023: Avoid construction of temporary strings in
`NumberWriter` for doubles.
-----------------------------------------------------------
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.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 43023: Avoid construction of temporary strings in
`NumberWriter` for doubles.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/43023/#comment178241>
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.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
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.
Posted by Neil Conway <ne...@gmail.com>.
> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 173
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line173>
> >
> > 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
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line162>
> >
> > 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
>
>
Re: Review Request 43023: Avoid construction of temporary strings in
`NumberWriter` for doubles.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 162
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line162>
> >
> > 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
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line173>
> >
> > 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.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 162
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line162>
> >
> > 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.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 173
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line173>
> >
> > 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.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 31, 2016, 1:43 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 173
> > <https://reviews.apache.org/r/43023/diff/1/?file=1227235#file1227235line173>
> >
> > 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.
>
> Michael Park wrote:
> 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?
I've committed this patch with `s/i/back/`. If this is not sufficient, please let me know and I'll follow-up in a separate patch.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43023/#review117131
-----------------------------------------------------------
On Feb. 1, 2016, 5:14 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> Bugs: MESOS-4566
> https://issues.apache.org/jira/browse/MESOS-4566
>
>
> Repository: mesos
>
>
> Description
> -------
>
> 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
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp f9d72243bcd5ae951ae1837fef0842915ff1e896
>
> 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.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/43023/#comment178222>
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)
<https://reviews.apache.org/r/43023/#comment178224>
I assume we have some evidence that this optimization is warranted?
3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 173)
<https://reviews.apache.org/r/43023/#comment178223>
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
>
>