mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 63368: Added MemoryProfiler class to Libprocess.
Date Tue, 10 Apr 2018 14:48:37 GMT


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


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