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