----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37540/#review98924 -----------------------------------------------------------
my main question is regarding division of responsibilities between the wrapper class and the process class. Looks like opening the fd and mmaping is done by the wrapper and the rest by the process. Can everything be done by the process? src/linux/perf.hpp (line 92) <https://reviews.apache.org/r/37540/#comment155724> const & src/linux/perf.hpp (line 95) <https://reviews.apache.org/r/37540/#comment155649> s/, returns/. Returns/ src/linux/perf.hpp (line 96) <https://reviews.apache.org/r/37540/#comment155650> s/,/;/ s/cgroups/cgroup names/ ? src/linux/perf.hpp (line 99) <https://reviews.apache.org/r/37540/#comment155725> const src/linux/perf.hpp (line 126) <https://reviews.apache.org/r/37540/#comment155726> const & src/linux/perf.cpp (line 508) <https://reviews.apache.org/r/37540/#comment155701> s/, after/; after/ src/linux/perf.cpp (line 562) <https://reviews.apache.org/r/37540/#comment155710> sharing the map between this class and the constituent process class is unfortunate. what happens if the process class deletes the pointer? is it possible to have the process class create and own the map? src/linux/perf.cpp (line 564) <https://reviews.apache.org/r/37540/#comment155702> don't think you need CHECK_NOTNULL here because you are setting 'process' variable right above. also indent by 2 spaces instead of 4. src/linux/perf.cpp (lines 605 - 610) <https://reviews.apache.org/r/37540/#comment155735> indent these by 4 spaces from the beginning of the column (i.e., 4 spaces from 'void'). src/linux/perf.cpp (line 628) <https://reviews.apache.org/r/37540/#comment155730> CHECK_NOTNULL(attr); ? src/linux/perf.cpp (line 660) <https://reviews.apache.org/r/37540/#comment155729> CHECK_NOTNULL(attr)? src/linux/perf.cpp (line 699) <https://reviews.apache.org/r/37540/#comment155713> The fd returned by os::open() above is the pid of the cgroup? It's worth adding a comment. src/linux/perf.cpp (line 746) <https://reviews.apache.org/r/37540/#comment155709> is there any reason why we want close() as part of the public API? sounds like users can simply call delete on the PerfEvent object? i would recommend removing this method or making it private. more importantly, what happens if close() is called twice? is that safe? src/linux/perf.cpp (lines 748 - 751) <https://reviews.apache.org/r/37540/#comment155721> can this be done by the process instead? src/linux/perf.cpp (line 754) <https://reviews.apache.org/r/37540/#comment155708> default is 'true'. so no need to explicitly specify it. - Vinod Kone On Sept. 4, 2015, 11:13 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37540/ > ----------------------------------------------------------- > > (Updated Sept. 4, 2015, 11:13 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-2769 > https://issues.apache.org/jira/browse/MESOS-2769 > > > Repository: mesos > > > Description > ------- > > Abstract Linux kernel perf event API and provide API to collect schedule > events. > > > Diffs > ----- > > src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5 > src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 > src/tests/containerizer/perf_tests.cpp > bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 > > Diff: https://reviews.apache.org/r/37540/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Cong Wang > >
