You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/07/02 00:44:35 UTC
Review Request 36114: perf: added another extract function to support
the new perf format after v3.12.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/
-----------------------------------------------------------
Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
Bugs: mesos-2834
https://issues.apache.org/jira/browse/mesos-2834
Repository: mesos
Description
-------
perf: added another extract function to support the new perf format after v3.12.
Diffs
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36114/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Chi Zhang <ch...@gmail.com>.
> On July 2, 2015, 12:52 a.m., Paul Brett wrote:
> > src/linux/perf.cpp, line 473
> > <https://reviews.apache.org/r/36114/diff/1/?file=997704#file997704line473>
> >
> > How about extract_post_linux_2_6_39 for the name?
I wanted it that too, but functions names need to be lowerCamerl per style guide.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90180
-----------------------------------------------------------
On July 1, 2015, 10:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 10:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: added another extract function to support the new perf format after v3.12.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90180
-----------------------------------------------------------
src/linux/perf.cpp (line 473)
<https://reviews.apache.org/r/36114/#comment143176>
How about extract_post_linux_2_6_39 for the name?
src/linux/perf.cpp (line 498)
<https://reviews.apache.org/r/36114/#comment143177>
extract_post_linux_3_12_0?
- Paul Brett
On July 1, 2015, 10:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 10:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: added another extract function to support the new perf format after v3.12.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Chi Zhang <ch...@gmail.com>.
> On July 2, 2015, 12:28 a.m., Ian Downes wrote:
> > src/linux/perf.cpp, line 473
> > <https://reviews.apache.org/r/36114/diff/1/?file=997704#file997704line473>
> >
> > Is there some way to have different extract functions without coding a single, specific kernel version in the name, e.g., even extractv1, extractv2, ... seems preferable
used extractv1.
I think I can take advantage of the virtual functions in OO to do that, but not sure it's worth the complication.
> On July 2, 2015, 12:28 a.m., Ian Downes wrote:
> > src/linux/perf.cpp, lines 531-532
> > <https://reviews.apache.org/r/36114/diff/1/?file=997704#file997704line531>
> >
> > I don't understand these conditions, e.g., surely
> > ```
> > (version > Version(3,12,0)) == (version >= Version(3,0,0) && version > Version(3, 12, 0)
> > ```
> > for all possible values of version, unless I'm not understanding how versions are compared...?
> >
> > For this type of selection I think it's clearest/simplest to start from the lowest in the ordering:
> > ```cpp
> > if (a < x) {
> > //
> > } else if (a < y) {
> > //
> > } else if (a < z) {
> > //
> > } else {
> > //
> > }
> > ```
Don't quite follow your comment.
normally, kernel should go from 2.6.39 to 3.0 directly, but some kernel does 2.6.40 for 3.0, i.e., 'uname -r' can't be trusted.
We care about two bands:
1. 3.0 - 3.12 (vanilla), or its equivalant 2.6.40 to 2.6.52 (customized) -> v1;
2. >3.12 or its equivalent > 2.5.52 -> v2.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90170
-----------------------------------------------------------
On July 1, 2015, 10:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 10:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: added another extract function to support the new perf format after v3.12.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90170
-----------------------------------------------------------
src/linux/perf.cpp (line 473)
<https://reviews.apache.org/r/36114/#comment143171>
Is there some way to have different extract functions without coding a single, specific kernel version in the name, e.g., even extractv1, extractv2, ... seems preferable
src/linux/perf.cpp (lines 522 - 523)
<https://reviews.apache.org/r/36114/#comment143164>
long ternary operations should be aligned to ?
```cpp
tokens.size() == 6 ? tokens[3]
: PIDS_KEY
```
src/linux/perf.cpp (lines 531 - 532)
<https://reviews.apache.org/r/36114/#comment143170>
I don't understand these conditions, e.g., surely
```
(version > Version(3,12,0)) == (version >= Version(3,0,0) && version > Version(3, 12, 0)
```
for all possible values of version, unless I'm not understanding how versions are compared...?
For this type of selection I think it's clearest/simplest to start from the lowest in the ordering:
```cpp
if (a < x) {
//
} else if (a < y) {
//
} else if (a < z) {
//
} else {
//
}
```
src/linux/perf.cpp (line 549)
<https://reviews.apache.org/r/36114/#comment143163>
Where is the variable `version` defined?
- Ian Downes
On July 1, 2015, 3:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 3:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: added another extract function to support the new perf format after v3.12.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Chi Zhang <ch...@gmail.com>.
> On July 2, 2015, 11:44 p.m., Ben Mahler wrote:
> > src/linux/perf.cpp, line 496
> > <https://reviews.apache.org/r/36114/diff/2/?file=998617#file998617line496>
> >
> > Sorry for the drive-by comment, but why not just have a single 'extract' that based on the 'Version' performs the necessary parsing?
> >
> > Exposing two 'extractv1' and 'extractv2' global functions seems a bit confusing (what are v1 and v2?). Would rather see them inlined, or you could have lambdas if you feel that they need to be self-contained.. :)
just to clarify, v1 and v2 are file static right now?
Yeah, I am not happy with the names either; I am fine putting them directly into one extract branched on version.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90294
-----------------------------------------------------------
On July 2, 2015, 11:40 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -----------------------------------------------------------
>
> (Updated July 2, 2015, 11:40 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: added another extract function to support the new perf format after v3.12.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90294
-----------------------------------------------------------
src/linux/perf.cpp (line 496)
<https://reviews.apache.org/r/36114/#comment143302>
Sorry for the drive-by comment, but why not just have a single 'extract' that based on the 'Version' performs the necessary parsing?
Exposing two 'extractv1' and 'extractv2' global functions seems a bit confusing (what are v1 and v2?). Would rather see them inlined, or you could have lambdas if you feel that they need to be self-contained.. :)
- Ben Mahler
On July 2, 2015, 11:40 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -----------------------------------------------------------
>
> (Updated July 2, 2015, 11:40 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: added another extract function to support the new perf format after v3.12.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to
support the new perf format after v3.12.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/
-----------------------------------------------------------
(Updated July 2, 2015, 11:40 p.m.)
Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
Bugs: mesos-2834
https://issues.apache.org/jira/browse/mesos-2834
Repository: mesos
Description
-------
perf: added another extract function to support the new perf format after v3.12.
Diffs (updated)
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36114/diff/
Testing
-------
Thanks,
Chi Zhang