mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 37540: Add perf event API
Date Tue, 15 Sep 2015 00:31:46 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message