----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63368/#review196894 -----------------------------------------------------------
3rdparty/libprocess/include/process/memory_profiler.hpp Lines 57 (patched) <https://reviews.apache.org/r/63368/#comment276817> When describing what a function does or returns, we use 3rd person present simple tense: activates, returns, generates,... Please adjust here and everywhere. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 87 (patched) <https://reviews.apache.org/r/63368/#comment276816> Why? What is this id about? Also please mention that `id` is optional and the last know will be returned if `id` is omitted. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 115 (patched) <https://reviews.apache.org/r/63368/#comment276820> Period at the end. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 132-133 (patched) <https://reviews.apache.org/r/63368/#comment276821> FYI: Fits one line. However, the comment seems incorrect: this function does not return any path, rather, it is a side effect on success. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 148 (patched) <https://reviews.apache.org/r/63368/#comment276825> If you have private fields and getters, make it a class. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 157 (patched) <https://reviews.apache.org/r/63368/#comment276828> backtick type and variable names and put a period at the end. 3rdparty/libprocess/src/memory_profiler.cpp Lines 75 (patched) <https://reviews.apache.org/r/63368/#comment276847> I'd kill these blank lines. 3rdparty/libprocess/src/memory_profiler.cpp Lines 197-240 (patched) <https://reviews.apache.org/r/63368/#comment276855> I would still merge these two functions. Since it is for internal use only, it is fine to expect the user to understand when to expect a previous value (looks like there is one such instance in the current code). How about this? ``` template<typename T> Result<T> writeJemallocSetting(const char* name, const T& value, bool requestPrevious) { if (!detectJemalloc()) { return Error(JEMALLOC_NOT_DETECTED_MESSAGE); } T previous; T* pprevious = requestPrevious ? &previous : nullptr; size_t size = sizeof(previous); size_t* pszie = requestPrevious ? &size : nullptr; int error = mallctl( name, pprevious, psize, const_cast<T*>(&value), sizeof(value)); if (error) { return Error(strings::format( "Couldn't write value %s for option %s: %s", value, name, ::strerror(error)).get()); } return requestPrevious ? previous : None(); } ``` And if you make `value` optional, you can have a single function plus maybe three syntactic sugar functions with descriptive names that call into the basic one. 3rdparty/libprocess/src/memory_profiler.cpp Lines 243-249 (patched) <https://reviews.apache.org/r/63368/#comment276831> I think it is safe to use this function because it is only accessed from `MemoryProfiler::DiskArtifact::path()` which is in turn only accessed from `MemoryProfiler` methods, which are always serialized. You should call this in a comment or even employ `process::Once` now to make sure it is thread-safe and hence future-proof. 3rdparty/libprocess/src/memory_profiler.cpp Lines 264 (patched) <https://reviews.apache.org/r/63368/#comment276819> Say "using std::string" in the beginning of the file and save hundreds of characters! 3rdparty/libprocess/src/memory_profiler.cpp Lines 301 (patched) <https://reviews.apache.org/r/63368/#comment276861> I'm not sure newlines in `Error` is good idea. You can easily do without. 3rdparty/libprocess/src/memory_profiler.cpp Lines 311 (patched) <https://reviews.apache.org/r/63368/#comment276857> I'd avoid defaults in general. Given the ordering error below, I'd probably reorder the arguments and remove the default here. 3rdparty/libprocess/src/memory_profiler.cpp Lines 342 (patched) <https://reviews.apache.org/r/63368/#comment276859> No periods in error messages, please! 3rdparty/libprocess/src/memory_profiler.cpp Lines 381-382 (patched) <https://reviews.apache.org/r/63368/#comment276866> Instead of trailing spaces, use leading spaces. This way it is easier to spot when a space is missing. Here and below. 3rdparty/libprocess/src/memory_profiler.cpp Lines 393-395 (patched) <https://reviews.apache.org/r/63368/#comment276864> Looks like you would need a multiline message here. Use commas to separate lines. Ditto for other longer help messages you have below. 3rdparty/libprocess/src/memory_profiler.cpp Lines 437-470 (patched) <https://reviews.apache.org/r/63368/#comment276867> Aint't it nice and tidy now? 3rdparty/libprocess/src/memory_profiler.cpp Lines 480 (patched) <https://reviews.apache.org/r/63368/#comment276869> Who is `WARN()`? s/as WARN()/at `WARNING` level 3rdparty/libprocess/src/memory_profiler.cpp Lines 523-524 (patched) <https://reviews.apache.org/r/63368/#comment276832> Please blank comment line (` // `) between `TODO`s, `NOTE`s and alike. 3rdparty/libprocess/src/memory_profiler.cpp Lines 534 (patched) <https://reviews.apache.org/r/63368/#comment276833> 4 spaces indent is only after `(`. I know it's hard to follow the 2 vs. 4 indent rule, but please comply. 3rdparty/libprocess/src/memory_profiler.cpp Lines 544-547 (patched) <https://reviews.apache.org/r/63368/#comment276835> I'm not sure I understand this part. This function is for internal use only, so why would we try to write something to the file and reuse the id? And if we do so, why is this not an error? 3rdparty/libprocess/src/memory_profiler.cpp Lines 578 (patched) <https://reviews.apache.org/r/63368/#comment276870> Please create a default constant at the top of the file for this. 3rdparty/libprocess/src/memory_profiler.cpp Lines 592-598 (patched) <https://reviews.apache.org/r/63368/#comment276872> Let's 1) Introduce constants for these values 2) Mention them in the related help message 3) Return `BadRequest` if duration is out of range 3rdparty/libprocess/src/memory_profiler.cpp Lines 608 (patched) <https://reviews.apache.org/r/63368/#comment276876> No need for this extra variable, see comments below. 3rdparty/libprocess/src/memory_profiler.cpp Lines 611 (patched) <https://reviews.apache.org/r/63368/#comment276873> I doubt this is correct English. Suggestions: "... is already active", "has been already activated". 3rdparty/libprocess/src/memory_profiler.cpp Lines 630 (patched) <https://reviews.apache.org/r/63368/#comment276874> Use `profilingTimer->timeout().remaining()` directly. 3rdparty/libprocess/src/memory_profiler.cpp Lines 632 (patched) <https://reviews.apache.org/r/63368/#comment276875> no need for this variable 3rdparty/libprocess/src/memory_profiler.cpp Lines 633 (patched) <https://reviews.apache.org/r/63368/#comment276878> `const` please. 3rdparty/libprocess/src/memory_profiler.cpp Lines 634 (patched) <https://reviews.apache.org/r/63368/#comment276877> use `profilingTimer->timeout().remaining().secs()` directly. 3rdparty/libprocess/src/memory_profiler.cpp Lines 636 (patched) <https://reviews.apache.org/r/63368/#comment276856> Swap the parameters ; ) 3rdparty/libprocess/src/memory_profiler.cpp Lines 650-653 (patched) <https://reviews.apache.org/r/63368/#comment276879> 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? 3rdparty/libprocess/src/memory_profiler.cpp Lines 664 (patched) <https://reviews.apache.org/r/63368/#comment276880> No space between `[]` and `()`. `{` stays on the previous line. Reference: https://mesos.apache.org/documentation/latest/c++-style-guide/ 3rdparty/libprocess/src/memory_profiler.cpp Lines 680 (patched) <https://reviews.apache.org/r/63368/#comment276881> 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. 3rdparty/libprocess/src/memory_profiler.cpp Lines 693 (patched) <https://reviews.apache.org/r/63368/#comment276849> It looks like you don't use the previous value, so why not using `writeJemallocSetting()` instead? 3rdparty/libprocess/src/memory_profiler.cpp Lines 709 (patched) <https://reviews.apache.org/r/63368/#comment276883> Please clarify which retries you mean. Maybe something like "If any error, e.g., writing the output file, happens after this point, it is not retried..." 3rdparty/libprocess/src/memory_profiler.cpp Lines 722 (patched) <https://reviews.apache.org/r/63368/#comment276886> const 3rdparty/libprocess/src/memory_profiler.cpp Lines 732 (patched) <https://reviews.apache.org/r/63368/#comment276885> No periods at the end : ) 3rdparty/libprocess/src/memory_profiler.cpp Lines 766 (patched) <https://reviews.apache.org/r/63368/#comment276887> Let's tell the user what the available id is. 3rdparty/libprocess/src/memory_profiler.cpp Lines 803 (patched) <https://reviews.apache.org/r/63368/#comment276888> `{` goes onto the previous line 3rdparty/libprocess/src/memory_profiler.cpp Lines 804-806 (patched) <https://reviews.apache.org/r/63368/#comment276890> Isn't it already covered with the check above? 3rdparty/libprocess/src/memory_profiler.cpp Lines 853-855 (patched) <https://reviews.apache.org/r/63368/#comment276891> Ditto. 3rdparty/libprocess/src/memory_profiler.cpp Lines 879 (patched) <https://reviews.apache.org/r/63368/#comment276818> `const string` 3rdparty/libprocess/src/memory_profiler.cpp Lines 897-903 (patched) <https://reviews.apache.org/r/63368/#comment276893> Period at the end. 3rdparty/libprocess/src/memory_profiler.cpp Lines 897-903 (patched) <https://reviews.apache.org/r/63368/#comment276894> 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); } ``` 3rdparty/libprocess/src/memory_profiler.cpp Lines 897 (patched) <https://reviews.apache.org/r/63368/#comment276895> Period at the end. 3rdparty/libprocess/src/memory_profiler.cpp Lines 937 (patched) <https://reviews.apache.org/r/63368/#comment276896> Same `{}` trick as above? 3rdparty/libprocess/src/memory_profiler.cpp Lines 957 (patched) <https://reviews.apache.org/r/63368/#comment276897> Same `{}` trick as above? - Alexander Rukletsov 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 > >
