You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/07/10 01:08:41 UTC
Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
> On July 10, 2015, 5:25 p.m., Ian Downes wrote:
> > src/linux/perf.cpp, lines 469-474
> > <https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line469>
> >
> > We should document this behavior at os::release() and move this there as an alternative.
> >
> > If it's kept in here then make static?
> >
> > Argument should be named 'version', not 'v'.
> >
> > Does it need to have such a long function name, particularly if it's local?
> >
> > Version canonicalize(const Version& version).
MESOS-3029 raised to move function to stout, but the solution is not that obvious since we could (1) modify os::release to return the canonical version but then it would not match uname which would be an issue if you used it to try and find something like a kernel module or (2) provide a separate function which returns canonical release but then there is more potential for using the wrong one or (3) provide version with a custom sort order so that the returned string is as reported by uname but Version(3.0.0) sorts as less that Version(2.6.41).
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91253
-----------------------------------------------------------
On July 10, 2015, 5:33 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 5:33 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
> On July 10, 2015, 5:25 p.m., Ian Downes wrote:
> > src/linux/perf.cpp, line 482
> > <https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line482>
> >
> > Suggest moving the TODO to here.
I'd like to keep it with the definition since usage is in multiple locations in the file.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91253
-----------------------------------------------------------
On July 10, 2015, 8:48 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:48 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91253
-----------------------------------------------------------
src/linux/perf.cpp (line 449)
<https://reviews.apache.org/r/36378/#comment144639>
Why a Try<> argument and then a CHECK()? The caller of os::release() should check the return so either check it in supported() or query and check it in _supported().
Also, should be const Version&.
src/linux/perf.cpp (lines 468 - 473)
<https://reviews.apache.org/r/36378/#comment144642>
We should document this behavior at os::release() and move this there as an alternative.
If it's kept in here then make static?
Argument should be named 'version', not 'v'.
Does it need to have such a long function name, particularly if it's local?
Version canonicalize(const Version& version).
src/linux/perf.cpp (line 481)
<https://reviews.apache.org/r/36378/#comment144544>
Suggest moving the TODO to here.
src/linux/perf.cpp (lines 484 - 485)
<https://reviews.apache.org/r/36378/#comment144545>
There's no map, it returns a single Sample?
src/linux/perf.cpp (lines 486 - 488)
<https://reviews.apache.org/r/36378/#comment144645>
This fits on a single line.
src/linux/perf.cpp (lines 490 - 492)
<https://reviews.apache.org/r/36378/#comment144546>
newlines, please ;-)
src/linux/perf.cpp (line 492)
<https://reviews.apache.org/r/36378/#comment144648>
Can you provide links to the documentation/commits that introduced these changes?
src/linux/perf.cpp (line 508)
<https://reviews.apache.org/r/36378/#comment144549>
Check how patch versions are handled is correct, e.g., this would match 3.12.1 (if it existed) when I think the change was in 3.13.0 so it should be >= 3.13.0
src/linux/perf.cpp (line 524)
<https://reviews.apache.org/r/36378/#comment144649>
newline
src/linux/perf.cpp (line 532)
<https://reviews.apache.org/r/36378/#comment144651>
const T&
src/linux/perf.cpp (line 536)
<https://reviews.apache.org/r/36378/#comment144550>
newline
- Ian Downes
On July 9, 2015, 4:08 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 4:08 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91355
-----------------------------------------------------------
src/linux/perf.cpp (line 468)
<https://reviews.apache.org/r/36378/#comment144721>
Get version number from 'perf --version' not os::release.
src/linux/perf.cpp (line 623)
<https://reviews.apache.org/r/36378/#comment144720>
We should get the version number from 'perf --version' not os::release in case a mismatched perf is installed.
- Paul Brett
On July 10, 2015, 8:48 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 8:48 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
(Updated July 16, 2015, 9:32 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Changes
-------
Removed the dependency on 36424
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs (updated)
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
(Updated July 13, 2015, 9:04 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
(Updated July 13, 2015, 9:02 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Changes
-------
Get version from 'perf --stat' not os::release.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs (updated)
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
(Updated July 10, 2015, 8:48 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Changes
-------
Fixed bug handling multiple counters in cgroups.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs (updated)
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
(Updated July 10, 2015, 6:16 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Changes
-------
Addressed review comments.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs (updated)
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/
-----------------------------------------------------------
(Updated July 10, 2015, 5:33 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Diffs (updated)
-----
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
Diff: https://reviews.apache.org/r/36378/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Paul Brett <pa...@twopensource.com>.
> On July 10, 2015, 3:11 a.m., Chi Zhang wrote:
> > src/linux/perf.cpp, line 449
> > <https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line449>
> >
> > Using Try as a function parameter feels a bit non-idiomatic to me, though I agree it's less code this way.
In other places I would agree with you, but in this case we should be able to test for os::release returning an error so the Try makes sense.
> On July 10, 2015, 3:11 a.m., Chi Zhang wrote:
> > src/linux/perf.cpp, lines 476-483
> > <https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line476>
> >
> > I think using a hashmap as an intermediate type is more flexisible.
> >
> > Right now, the operation after 'extract' is the same for all versions, but we can make it version-dependent too in the future to support e.g. unit, running time, etc.
Disagree. The Sample struct is local to this file, so it is very easy to change. Using a hashmap introduces the possibility for runtime errors which are caught at compile time this way.
> On July 10, 2015, 3:11 a.m., Chi Zhang wrote:
> > src/linux/perf.cpp, lines 462-474
> > <https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line462>
> >
> > This is great abstraction; I agree we should put it into stout. (maybe even have os::release just return canonicalized linux version?)
I will raise this as a separate ticket. Making os::release return the canonicalized linux version introduces the potential confusion that uname would no longer match os::release.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91262
-----------------------------------------------------------
On July 9, 2015, 11:08 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 11:08 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36378: Refactor Linux Performance monitor to handle
changing 'perf stat' output versions depending on kernel version.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91262
-----------------------------------------------------------
Thanks for taking on this for me!
src/linux/perf.cpp (line 449)
<https://reviews.apache.org/r/36378/#comment144554>
Using Try as a function parameter feels a bit non-idiomatic to me, though I agree it's less code this way.
src/linux/perf.cpp (line 459)
<https://reviews.apache.org/r/36378/#comment144555>
2 lines above? other places too.
src/linux/perf.cpp (lines 461 - 473)
<https://reviews.apache.org/r/36378/#comment144557>
This is great abstraction; I agree we should put it into stout. (maybe even have os::release just return canonicalized linux version?)
src/linux/perf.cpp (lines 475 - 482)
<https://reviews.apache.org/r/36378/#comment144558>
I think using a hashmap as an intermediate type is more flexisible.
Right now, the operation after 'extract' is the same for all versions, but we can make it version-dependent too in the future to support e.g. unit, running time, etc.
- Chi Zhang
On July 9, 2015, 11:08 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 11:08 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>