> On Oct. 31, 2017, 1:53 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/include/process/memory_profiler.hpp > > Lines 26 (patched) > > <https://reviews.apache.org/r/63368/diff/1/?file=1870336#file1870336line26> > > > > I realize you're probably copying the style of the cpu profiler, but I > > added that long ago and would instead write it using a Process wrapper now, > > so just the MemoryProfiler in the header and the MemoryProfilerProcess in > > the .cpp. > > > > Also, some documenation in the header file would be great! In > > particular, what this is (some endpoints that provide an API for memory > > profiling using jemalloc?), and also some of the cross-function semantics, > > like lifecycle (start -> get profile -> get statistics -> ... -> stop).
I've experimented a bit with this, but as far as I can see the benefits of introducing a `MemoryProfilerProcess` would be questionable in this particular case: Right now this class is created once, globally, and only interface are the http endpoints. Introducing a separate `MemoryProfilerProcess` would mean triplicating the entire interface, for a functionality that is not expected to be used. Furthermore, for some functions like `statistics()`, the natural return type would be `JSON::Object MemoryProfiler::statistics()` - but the HTTP endpoint ultimately needs to return a string, so we would have to parse a JSON struct just to immediately serialize it again, which seems needlessly convoluted. So I would suggest following the precedence of the other libprocess-global processes (`Profiler`, `Logging`, etc.) and leaving it as it is. As to the documentation, it was greatly expanded. > On Oct. 31, 2017, 1:53 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/include/process/memory_profiler.hpp > > Lines 49-54 (patched) > > <https://reviews.apache.org/r/63368/diff/1/?file=1870336#file1870336line49> > > > > Does this drop down a file? Is it possible to avoid that? Who will > > clean up this file? Dropping this, since the function does not exist anymore in this form. In general, dropping files is unavoidable as it is the only API provided by Mesos. For the initial version, they're not cleared at all (it's at most 3 files, and they're only added when profiling is actually used), later on we would probably want to add an `atexit()`-handler. - Benno ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63368/#review189680 ----------------------------------------------------------- On Feb. 21, 2018, 10:41 a.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63368/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2018, 10:41 a.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 > 42e7adf740b234ebf23d2dcb71440749c0ed87ec > > > Diff: https://reviews.apache.org/r/63368/diff/6/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
