> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/include/process/memory_profiler.hpp > > Lines 104-113 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991764#file1991764line104> > > > > Does this class have to be nested? How about making it > > `jemalloc::State` in the same file?
None of them technically *need* to be nested, but it feels a bit cleaner to not litter the `process`-namespace unnecessarily. It also seems more consistent to me to treat all three classes as similar as possible. > On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/include/process/memory_profiler.hpp > > Lines 117-120 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991764#file1991764line117> > > > > 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? I'm dropping this as I'm now explicitly passing a pointer, but the reason it was safe was actually another one: `ProfilingRun` was a member of `MemoryProfiler`, so it will be destroyed before the base class, no matter whether it's global or not. > On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/include/process/memory_profiler.hpp > > Lines 120-121 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991764#file1991764line120> > > > > s/class/struct/ > > rm public: Dropping this because it violates our style guide (https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes) > On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/CMakeLists.txt > > Lines 47 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991766#file1991766line47> > > > > Please add *.hpp as well. I think if we do this, it should be part of a separate patch series - right now, none of the public headers are included in the `CMakeLists.txt`. > On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/memory_profiler.cpp > > Lines 794 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991767#file1991767line794> > > > > So if there is an error reading the jemalloc setting, the binary > > crashes? No, only if the setting is read successfully and it is still active. > On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/memory_profiler.cpp > > Lines 797 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991767#file1991767line797> > > > > If we come here via `/stop` endpoint, can the still running timer > > occasionally stop the next profiling run? Urgh, I'm not sure how I did forget about this one again, especially since the original motivation for introducing `ProfilingRun` was to avoid exactly this race :D > On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/process.cpp > > Lines 256 (patched) > > <https://reviews.apache.org/r/63368/diff/8/?file=1991768#file1991768line256> > > > > 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. I'm not sure, keep in mind that the actual "feature" is hidden anyways behind the `--enable-jemalloc-allocator` configuration setting, which is off by default. Without this, and assuming the user doesn't manually link against jemalloc, all endpoints will just return an error message saying that jemalloc couldn't be detected. - Benno ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63368/#review200633 ----------------------------------------------------------- On April 10, 2018, 2:39 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63368/ > ----------------------------------------------------------- > > (Updated April 10, 2018, 2:39 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/Makefile.am > cd2c3bc62df8de5b50ec2fa830b3e2634ba11e28 > 3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION > 3rdparty/libprocess/include/process/process.hpp > c36df991b6a2c120ab0562e8ff907f9fbf8630d1 > 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/9/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
