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 Wed, 21 Feb 2018 10:42:54 GMT


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


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