----------------------------------------------------------- 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 ``` [email protected]: ~/Projects/mesos.build $ echo $CXX ccache g++ -Qunused-arguments -fcolor-diagnostics -fvisibility-inlines-hidden -Wno-deprecated-declarations [email protected]: ~/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 > >
