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, 07 Feb 2018 17:23:13 GMT


> On Feb. 6, 2018, 5:48 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 630 (patched)
> > <https://reviews.apache.org/r/63368/diff/3/?file=1951296#file1951296line630>
> >
> >     Use `profilingTimer->timeout().remaining()` directly.

There are two issues with that:
1) The redirectTime is currently selected to be 2 seconds shorter than the time remaining
in the profiling timer, so by default it is the redirect that is stopping the profiling
2) Aesthetically, if a user is selecting `duration=5secs` it seems odd to display `You will
be redirected in 4.99993 seconds`.


> On Feb. 6, 2018, 5:48 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 650-653 (patched)
> > <https://reviews.apache.org/r/63368/diff/3/?file=1951296#file1951296line650>
> >
> >     Why do you need to check if you can read the key? Can this be omitted or maybe
moved to the bottom of the function?

The key must be read *before* we stop profiling, since we are interested in the previous value
of this setting. This should realistically never fail, so if it does the circumstances are
so weird that we probably shouldn't continue working with this jemalloc (e.g. the user is
using a custom-compiled version where this setting was removed/renamed)


> On Feb. 6, 2018, 5:48 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 680 (patched)
> > <https://reviews.apache.org/r/63368/diff/3/?file=1951296#file1951296line680>
> >
> >     I think the right criteria here is (was_active && not_active_now). If
you want to fo with a single read of a jemalloc setting, then still use the value right after
obtaining it.

We only get here if `stopAndGenerateRawProfile()` did return success, so `not_active_now`
should always be true.


> On Feb. 6, 2018, 5:48 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 897-903 (patched)
> > <https://reviews.apache.org/r/63368/diff/3/?file=1951296#file1951296line897>
> >
> >     Maybe this instead?
> >     ```
> >       // State unrelated to jemalloc.
> >       JSON::Object state;
> >       {
> >         JSON::Object profilerState;
> >         profilerState.values["jemalloc_detected"] = detected;
> >         profilerState.values["profiling_active"] = heapProfilingActive;
> >     
> >         state.values["memory_profiler"] = std::move(profilerState);
> >       }
> >     ```

I've rearranged it, but I'm not entirely sure about this because this pattern starts nesting
in the later parts of this function.


- Benno


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63368/#review196894
-----------------------------------------------------------


On Feb. 6, 2018, 5:45 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2018, 5:45 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 3071b7ce2b82a4ce0ea62e4d2b3518a6f5114803 
>   3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 8661706cb058efb26f5bfbcc84972f9930d3670f

>   3rdparty/libprocess/src/CMakeLists.txt f002c157dc2ca64da66bc4e61f5095f2b533ae1f 
>   3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp ba9bc291bb6741e32b3a066fe90771311d21852a 
> 
> 
> Diff: https://reviews.apache.org/r/63368/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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