----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37540/#review97548 -----------------------------------------------------------
src/linux/perf.hpp (line 71) <https://reviews.apache.org/r/37540/#comment153551> we typically name the internal process with a process suffix. s/Listener/Process/ src/linux/perf.hpp (line 77) <https://reviews.apache.org/r/37540/#comment153530> s/.././ src/linux/perf.hpp (line 78) <https://reviews.apache.org/r/37540/#comment153531> put the argument on the next line. src/linux/perf.hpp (lines 84 - 89) <https://reviews.apache.org/r/37540/#comment153532> Do we need this overload considering users can call the overload below? src/linux/perf.hpp (lines 88 - 89) <https://reviews.apache.org/r/37540/#comment153535> swap the arguments. src/linux/perf.hpp (lines 95 - 96) <https://reviews.apache.org/r/37540/#comment153534> Swap the arguments. src/linux/perf.hpp (line 111) <https://reviews.apache.org/r/37540/#comment153536> argument on next line. src/linux/perf.hpp (line 119) <https://reviews.apache.org/r/37540/#comment153552> We use "_" prefix for continuation. Since this is just a private overload, I would remove "_". src/linux/perf.cpp (lines 471 - 484) <https://reviews.apache.org/r/37540/#comment153575> What is this for? Comments? src/linux/perf.cpp (lines 496 - 497) <https://reviews.apache.org/r/37540/#comment153577> kill this? src/linux/perf.cpp (lines 501 - 503) <https://reviews.apache.org/r/37540/#comment153576> kill this? src/linux/perf.cpp (lines 514 - 516) <https://reviews.apache.org/r/37540/#comment153580> swap the order. src/linux/perf.cpp (line 522) <https://reviews.apache.org/r/37540/#comment153579> why inline? should this be static? also, why is this defined here instead of outside the class as you did for other methods? src/linux/perf.cpp (line 541) <https://reviews.apache.org/r/37540/#comment153582> typically constructor and destructor are defined first. src/linux/perf.cpp (lines 547 - 549) <https://reviews.apache.org/r/37540/#comment153601> combine these. int ret = ... src/linux/perf.cpp (lines 574 - 576) <https://reviews.apache.org/r/37540/#comment153602> indent arguments. src/linux/perf.cpp (line 577) <https://reviews.apache.org/r/37540/#comment153603> new line. src/linux/perf.cpp (lines 622 - 623) <https://reviews.apache.org/r/37540/#comment153600> kill this. see below. src/linux/perf.cpp (line 627) <https://reviews.apache.org/r/37540/#comment153599> you can just use an initializer list here {cpu} src/linux/perf.cpp (line 630) <https://reviews.apache.org/r/37540/#comment153598> new line please. src/linux/perf.cpp (line 720) <https://reviews.apache.org/r/37540/#comment153597> s/pgsz/pageSize/ src/linux/perf.cpp (line 742) <https://reviews.apache.org/r/37540/#comment153585> these should be defined in the following order ``` read() _read() __read() ``` src/linux/perf.cpp (line 746) <https://reviews.apache.org/r/37540/#comment153595> s/pgsz/pageSize/ s/pgmsk/pageMask/ ? src/linux/perf.cpp (line 790) <https://reviews.apache.org/r/37540/#comment153593> why is __read() a separate function? can you just pull the logic down here? src/linux/perf.cpp (line 817) <https://reviews.apache.org/r/37540/#comment153587> put the .onReady() on the next line. src/linux/perf.cpp (line 818) <https://reviews.apache.org/r/37540/#comment153592> who can discard this future? src/tests/containerizer/perf_tests.cpp (line 126) <https://reviews.apache.org/r/37540/#comment153538> s/inherit/Inherit/ src/tests/containerizer/perf_tests.cpp (line 128) <https://reviews.apache.org/r/37540/#comment153539> argument on next line. Can you add a comment on what this is trying to create? Not clear at all based on the arguments. src/tests/containerizer/perf_tests.cpp (line 146) <https://reviews.apache.org/r/37540/#comment153540> ditto. argument on next line. also, needs a comment on what this is creating. src/tests/containerizer/perf_tests.cpp (line 151) <https://reviews.apache.org/r/37540/#comment153542> ASSERT_SOME(event); src/tests/containerizer/perf_tests.cpp (lines 156 - 157) <https://reviews.apache.org/r/37540/#comment153548> s/context/Context/ Can you add a better comment on what you are tryign to do here. "Causing 2 context switches so that..? - Vinod Kone On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37540/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2015, 10:16 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 c44445630118f4858b1a805f25a2db7a24ca0989 > src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea > src/tests/containerizer/perf_tests.cpp > 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 > > Diff: https://reviews.apache.org/r/37540/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Cong Wang > >
