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




3rdparty/libprocess/Makefile.am
Lines 208 (patched)
<https://reviews.apache.org/r/63368/#comment281411>

    Add *.hpp file as well please.



3rdparty/libprocess/Makefile.am
Lines 301 (patched)
<https://reviews.apache.org/r/63368/#comment281412>

    I don't see this file in this diff.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 24 (patched)
<https://reviews.apache.org/r/63368/#comment281423>

    also `try.hpp`, `nothing.hpp`



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 104 (patched)
<https://reviews.apache.org/r/63368/#comment281410>

    Lbrace onto the next line please. Here and below.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 104-113 (patched)
<https://reviews.apache.org/r/63368/#comment281415>

    Does this class have to be nested? How about making it `jemalloc::State` in 
the same file?



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 117-120 (patched)
<https://reviews.apache.org/r/63368/#comment281422>

    Can you please mention here that it is safe to cache `MemoryProfiler` 
because it is destructed on process termination? Or, if you prefer to capture a 
lambda, to warn that users must guarantee that it is safe to invoke the lambda 
at a future point of time?



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 120-121 (patched)
<https://reviews.apache.org/r/63368/#comment281417>

    s/class/struct/
    rm public:



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 127 (patched)
<https://reviews.apache.org/r/63368/#comment281420>

    Instead of capturing the reference, how about passing `MemoryProfiler*` 
into both c-tor and `extend()`? Or maybe even to pass an `onFinished` lambda 
into `ProfilingRun` at construction and let its user define the action?



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 160 (patched)
<https://reviews.apache.org/r/63368/#comment281424>

    Why leading underscore?



3rdparty/libprocess/src/CMakeLists.txt
Lines 47 (patched)
<https://reviews.apache.org/r/63368/#comment281413>

    Please add *.hpp as well.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 41-45 (patched)
<https://reviews.apache.org/r/63368/#comment281428>

    Please mention the autostop and the collection time concept.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 49 (patched)
<https://reviews.apache.org/r/63368/#comment281426>

    either "is created" or "are accessed" says an ESL in me.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 53 (patched)
<https://reviews.apache.org/r/63368/#comment281427>

    Please backtick type, function, variable, etc names in comments.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 58-59 (patched)
<https://reviews.apache.org/r/63368/#comment281429>

    Why new line?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 59-63 (patched)
<https://reviews.apache.org/r/63368/#comment281430>

    Apparently, this does not work build on Mac
    ```
    alex@alexr.local: ~/Projects/mesos.build $ echo $CXX   
    ccache g++ -Qunused-arguments -fcolor-diagnostics 
-fvisibility-inlines-hidden -Wno-deprecated-declarations
    
    alex@alexr.local: ~/Projects/mesos.build $ g++ --version
    Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr 
--with-gxx-include-dir=/usr/include/c++/4.2.1
    Apple LLVM version 9.0.0 (clang-900.0.39.2)
    ```
    Breaking build is not an option, if it is hard tofix. let's disable the 
code on Mac.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 96 (patched)
<https://reviews.apache.org/r/63368/#comment281432>

    Do you mean `--memory_profiling` flag of the binary or 
`--enable-jemalloc-allocator`? Can you please look through all comments and 
docs and groom the terminalogy and flag/parameter names?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 517-520 (patched)
<https://reviews.apache.org/r/63368/#comment281418>

    This is racy. The timer may elapse and try to invoke the callback, which 
uses a field `this->profiler` of a not yet initialized instance `this`.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 646 (patched)
<https://reviews.apache.org/r/63368/#comment281414>

    remove `principal`



3rdparty/libprocess/src/memory_profiler.cpp
Lines 690 (patched)
<https://reviews.apache.org/r/63368/#comment281434>

    Let's return `Conflict`. I doubt we should return 2** if we can't provide 
the user with the capability they request and will return `Bad Request` for 
`/stop`.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 698 (patched)
<https://reviews.apache.org/r/63368/#comment281435>

    space before After



3rdparty/libprocess/src/memory_profiler.cpp
Lines 703 (patched)
<https://reviews.apache.org/r/63368/#comment281436>

    Can you please explain here in the comment why you add `0.5`?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 773 (patched)
<https://reviews.apache.org/r/63368/#comment281437>

    ?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 794 (patched)
<https://reviews.apache.org/r/63368/#comment281438>

    So if there is an error reading the jemalloc setting, the binary crashes?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 797 (patched)
<https://reviews.apache.org/r/63368/#comment281440>

    If we come here via `/stop` endpoint, can the still running timer 
occasionally stop the next profiling run?



3rdparty/libprocess/src/process.cpp
Lines 256 (patched)
<https://reviews.apache.org/r/63368/#comment281425>

    Let's make it `false` by default for now since it is an experimental 
feature, and turn it on after some time, ideally after some production testing 
together with feature graduation.


- Alexander Rukletsov


On April 3, 2018, 4:29 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> -----------------------------------------------------------
> 
> (Updated April 3, 2018, 4:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This class exposes profiling functionality of jemalloc memory allocator
> when it is detected to be the memory allocator of the current process.
> 
> In particular, it gives developers an easy method to collect and
> access heap profiles which report which pieces of code were
> responsible for allocating memory.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am c895d3ac7b9cc5ffd6c8b57ff24def866eb0213d 
>   3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 8661706cb058efb26f5bfbcc84972f9930d3670f 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> 9eb37465cd86f408d69f5f98fb76c4f4b93b9acd 
> 
> 
> Diff: https://reviews.apache.org/r/63368/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Reply via email to