mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 63368: Added MemoryProfiler class to Libprocess.
Date Tue, 06 Feb 2018 17:48:39 GMT

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


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