You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2014/05/14 20:16:50 UTC

Review Request 21443: Support for running `perf stat`.

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

Review request for mesos and Benjamin Hindman.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs
-----

  src/Makefile.am 12374c4a87836c160a7050c37ff7fec544b724e1 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review44094
-----------------------------------------------------------


Bad patch!

Reviews applied: [21443]

Failed command: git apply --index 21443.patch

Error:
 error: patch failed: src/Makefile.am:253
error: src/Makefile.am: patch does not apply


- Mesos ReviewBot


On May 14, 2014, 10:05 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 10:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 12374c4a87836c160a7050c37ff7fec544b724e1 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/isolator_tests.cpp b0eff5721b6ea4dcee8ff3748a2a11ade809e30e 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.

> On May 15, 2014, 12:21 p.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 232
> > <https://reviews.apache.org/r/21443/diff/2/?file=581929#file581929line232>
> >
> >     Doesn't look like this is being used anywhere? Kill it.

We should still read from stderr to avoid potential to block the process on the pipe but changed to not store the future.


- Ian


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


On May 29, 2014, 2:25 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:25 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review43141
-----------------------------------------------------------



src/linux/perf.hpp
<https://reviews.apache.org/r/21443/#comment77156>

    2 lines between declarations.



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment77160>

    end with period.



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment77161>

    If it wraps within 80, you should do
    
    Try<double> number =
      (value == ......................



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment77162>

    Doesn't look like this is being used anywhere? Kill it.



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment77163>

    Kill the else.
    
    Just do 
    
    if (parse.isErorr()) {
      ...
      return;
    }
    
    ...
    ...



src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/21443/#comment77164>

    end with period.



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/21443/#comment77165>

    Hmm..Does this belong to this review?


- Vinod Kone


On May 14, 2014, 10:05 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 10:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 12374c4a87836c160a7050c37ff7fec544b724e1 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/isolator_tests.cpp b0eff5721b6ea4dcee8ff3748a2a11ade809e30e 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review45065
-----------------------------------------------------------


Bad patch!

Reviews applied: [21443]

Failed command: git apply --index 21443.patch

Error:
 error: patch failed: src/Makefile.am:287
error: src/Makefile.am: patch does not apply


- Mesos ReviewBot


On June 9, 2014, 5:09 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 9, 2014, 5:09 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review45662
-----------------------------------------------------------

Ship it!


Ship It!

- Vinod Kone


On June 13, 2014, 9:39 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 13, 2014, 9:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated June 13, 2014, 2:39 p.m.)


Review request for mesos and Vinod Kone.


Changes
-------

Addressed Vinod's comments.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs (updated)
-----

  src/Makefile.am c91b438c3401ff815116d6faab4f0a34bc0fe3fd 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review45558
-----------------------------------------------------------



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment80439>

    const?



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment80444>

    i still think an empty string is a bit weird. you didn't want to use something like "__pids__" because a cgroup can have that name? Mesos conainerizer uses UUIDs so it shouldn't be a problem, but I guess EC can create arbitrary cgroup names? can a cgroup name be ""?
    
    either way, extract this into a constant (PIDS_KEY?).



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment80440>

    const?


- Vinod Kone


On June 12, 2014, 9:51 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 12, 2014, 9:51 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review45588
-----------------------------------------------------------


Bad patch!

Reviews applied: [21443]

Failed command: git apply --index 21443.patch

Error:
 error: patch failed: src/Makefile.am:287
error: src/Makefile.am: patch does not apply


- Mesos ReviewBot


On June 12, 2014, 9:51 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 12, 2014, 9:51 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated June 12, 2014, 9:51 p.m.)


Review request for mesos and Vinod Kone.


Changes
-------

i'll shepherd this -- vinod.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs
-----

  src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review45441
-----------------------------------------------------------


Bad patch!

Reviews applied: [21442, 21443]

Failed command: git apply --index 21443.patch

Error:
 error: patch failed: src/Makefile.am:287
error: src/Makefile.am: patch does not apply


- Mesos ReviewBot


On June 11, 2014, 5:29 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 5:29 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated June 11, 2014, 10:29 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
-------

Added back depends on.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs
-----

  src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated June 8, 2014, 10:09 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
-------

Rebased and minor clean ups.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs (updated)
-----

  src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.

> On June 6, 2014, 12:14 p.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 332
> > <https://reviews.apache.org/r/21443/diff/2-5/?file=581929#file581929line332>
> >
> >     s/use ""/use ""  as key/ ?
> >     
> >     also, i don't understand the semantics of ""? why is this not an Option?

Option<T> isn't hashable so it uses "" when it's for pids, rather than the cgroup name.


> On June 6, 2014, 12:14 p.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 433
> > <https://reviews.apache.org/r/21443/diff/2-5/?file=581929#file581929line433>
> >
> >     can this be pid or stringify(pids) instead of "" ?

No, it needs to be something known that can't match any cgroup name.


- Ian


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


On June 3, 2014, 1:45 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 3, 2014, 1:45 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review44951
-----------------------------------------------------------



src/linux/perf.hpp
<https://reviews.apache.org/r/21443/#comment79534>

    s/Expose/NOTE: Exposed/



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment79540>

    s/,/;/



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment79541>

    s/use ""/use ""  as key/ ?
    
    also, i don't understand the semantics of ""? why is this not an Option?



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment79539>

    Taking a future as an argument here seems weird. Why not take the hashmap directly?
    AFAICT, this is called with a .then().



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment79542>

    can this be pid or stringify(pids) instead of "" ?


- Vinod Kone


On June 3, 2014, 8:45 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 3, 2014, 8:45 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated June 3, 2014, 1:45 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
-------

Cleaned up the API per Ben's suggestions


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs (updated)
-----

  src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.

> On May 30, 2014, 12:35 p.m., Ben Mahler wrote:
> > src/linux/perf.hpp, lines 72-74
> > <https://reviews.apache.org/r/21443/diff/4/?file=599352#file599352line72>
> >
> >     Should we put this in an 'internal' namespace to at least make it harder to get at for callers?
> >     
> >     Let's document what it returns, that is, what are the keys?

Originally it was but I pulled it out to expose for testing. Is there a nicer way to keep it accessible for testing but otherwise inaccessible?


- Ian


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


On June 3, 2014, 1:45 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated June 3, 2014, 1:45 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review44409
-----------------------------------------------------------


Hey Ian, took a look at the API being exposed and a quick look through the implementation.


src/linux/perf.hpp
<https://reviews.apache.org/r/21443/#comment78783>

    What is being returned? I can't intuit what the keys would be in this map without looking at the implementation.
    
    Can you add comments here and then if we don't need to return maps then can we simplify this and just directly return the PerfStatistics?



src/linux/perf.hpp
<https://reviews.apache.org/r/21443/#comment78785>

    What about:
    
    // Returns whether the events are valid.
    bool valid(const set<string>& events);
    
    This then seems to read a bit better to callers:
    
    if (perf::valid(events)) {
    
    }
    
    vs.
    
    if (perf::validate(events)) {
    
    }
    
    Because we're asking a question about the events it seems a bit easier to read it as 'valid' or 'isValid'. 'validate' sounds like an action more so than a question.



src/linux/perf.hpp
<https://reviews.apache.org/r/21443/#comment78786>

    Should we put this in an 'internal' namespace to at least make it harder to get at for callers?
    
    Let's document what it returns, that is, what are the keys?



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment78788>

    Maybe a comment as to why the _nested_ loop here is needed?



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment78789>

    Why not just group the comments and statements:
    
    // Start reading from stdout and stderr now. We don't use
    // stderr but must read from it to avoid blocking the child.
    output.push_back(process::io::read(perf.get().out()));
    output.push_back(process::io::read(perf.get().err()));



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment78790>

    The future failure message should be enough here, since this is designed as a library we should avoid logging internally in a redundant manner with what we send back to the caller.



src/linux/perf.cpp
<https://reviews.apache.org/r/21443/#comment78787>

    


- Ben Mahler


On May 29, 2014, 10:07 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 10:07 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/#review44317
-----------------------------------------------------------


Bad patch!

Reviews applied: [21443]

Failed command: git apply --index 21443.patch

Error:
 error: patch failed: src/Makefile.am:258
error: src/Makefile.am: patch does not apply


- Mesos ReviewBot


On May 29, 2014, 10:07 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21443/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 10:07 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-1278
>     https://issues.apache.org/jira/browse/MESOS-1278
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Perf can be run against either a set of pids or a perf_event cgroup.
> 
> I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
>   src/linux/perf.hpp PRE-CREATION 
>   src/linux/perf.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
>   src/tests/perf_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21443/diff/
> 
> 
> Testing
> -------
> 
> Added tests for pids and cgroups.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated May 29, 2014, 3:07 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Added support for sampling a set of cgroups.

Added tests for parsing out perf output.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs (updated)
-----

  src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated May 29, 2014, 2:25 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Amended to support running perf stat against a set of a cgroups.

Added tests for parsing perf stat output.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs (updated)
-----

  src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes

Re: Review Request 21443: Support for running `perf stat`.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21443/
-----------------------------------------------------------

(Updated May 14, 2014, 3:05 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Fixed some style issues.


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


Repository: mesos-git


Description
-------

Perf can be run against either a set of pids or a perf_event cgroup.

I decided to parse the perf output into the protobuf directly, rather than via JSON, because it was simpler to handle output like <not supported> and <not counted>. Plus JSON's floating point numbers cannot fully represent uint64.


Diffs (updated)
-----

  src/Makefile.am 12374c4a87836c160a7050c37ff7fec544b724e1 
  src/linux/perf.hpp PRE-CREATION 
  src/linux/perf.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 
  src/tests/isolator_tests.cpp b0eff5721b6ea4dcee8ff3748a2a11ade809e30e 
  src/tests/perf_tests.cpp PRE-CREATION 

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


Testing
-------

Added tests for pids and cgroups.


Thanks,

Ian Downes