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 Fri, 26 Jan 2018 17:31:51 GMT

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




3rdparty/libprocess/Makefile.am
Lines 213 (patched)
<https://reviews.apache.org/r/63368/#comment275910>

    Please tabs



3rdparty/libprocess/Makefile.am
Lines 303 (patched)
<https://reviews.apache.org/r/63368/#comment275911>

    Please tabs



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 36 (patched)
<https://reviews.apache.org/r/63368/#comment275912>

    No need to use the FQN here.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 51-52 (patched)
<https://reviews.apache.org/r/63368/#comment275914>

    "How long"... what? Can you please be more verbose here?
    Also you mean request parameters, can you say this explicitly?



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 57-58 (patched)
<https://reviews.apache.org/r/63368/#comment275926>

    Blank comment line in-between, please



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 64-66 (patched)
<https://reviews.apache.org/r/63368/#comment275928>

    Please write a leading comment explaining what these functions return, i.e., last _finished_
state.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 67 (patched)
<https://reviews.apache.org/r/63368/#comment275927>

    No need for process prefix, here and below.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 81 (patched)
<https://reviews.apache.org/r/63368/#comment275933>

    What kind of statistics is this? Is this related to the last / active state? Or is it
accumulated across all states since process launch? Can you please elaborate?



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 104 (patched)
<https://reviews.apache.org/r/63368/#comment275980>

    Let's call this guy `stopAndGenerateRawProfile()` for clarity.



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 124 (patched)
<https://reviews.apache.org/r/63368/#comment275925>

    ~~One shot one kill~~one line one declaration, please.
    
    Any reason to keep these fellas as `Try`s?
    
    Please also expand comments on these fields. For example, 
    ```
    // A timestamp of a last successful raw dump.
    Option<time_t> rawId;
    ```



3rdparty/libprocess/include/process/memory_profiler.hpp
Lines 130 (patched)
<https://reviews.apache.org/r/63368/#comment275918>

    Let's make it `const`. You will likely need a static or free function that read the env
var.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 52 (patched)
<https://reviews.apache.org/r/63368/#comment275959>

    Please backtick type, variable, class, and function names in comments.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 198-240 (patched)
<https://reviews.apache.org/r/63368/#comment275960>

    Can we combine these two with in a `Result<T> updateJemallocSetting(const char*
name, const T& value)`?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 245 (patched)
<https://reviews.apache.org/r/63368/#comment275963>

    Here you convert `string` to `Path` in order to convert it back at a call site. I suggest
to make `getRawProfilePath()` return `string.
    
    Also, there is no need in leading slashes.
    
    And since you already have a section for literals, maybe put file names there?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 261-271 (patched)
<https://reviews.apache.org/r/63368/#comment275966>

    It's not obvious that updating a setting triggers a dump. Can you please leave a note
here?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 275 (patched)
<https://reviews.apache.org/r/63368/#comment275967>

    One shot one...



3rdparty/libprocess/src/memory_profiler.cpp
Lines 313 (patched)
<https://reviews.apache.org/r/63368/#comment275968>

    Why newline?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 326-330 (patched)
<https://reviews.apache.org/r/63368/#comment275971>

    Formatting.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 334 (patched)
<https://reviews.apache.org/r/63368/#comment275970>

    Let's make it a lambda in `start()`



3rdparty/libprocess/src/memory_profiler.cpp
Lines 347 (patched)
<https://reviews.apache.org/r/63368/#comment275969>

    Let's make it a lambda in `stop()`.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 355-357 (patched)
<https://reviews.apache.org/r/63368/#comment275972>

    For clarity, let's instead make it 
    ```
    Option<time_t> extractIdFromRequest(const process::http::Request& request)
    ```
    Then at the call site you can say:
    `extractIdFromRequest(r).getOrElse(fallback.getOrElse(0))`



3rdparty/libprocess/src/memory_profiler.cpp
Lines 361 (patched)
<https://reviews.apache.org/r/63368/#comment275974>

    `strtol`?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 372 (patched)
<https://reviews.apache.org/r/63368/#comment275975>

    Consider using an alias for `process::http` in this file.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 382 (patched)
<https://reviews.apache.org/r/63368/#comment275976>

    kill "end"



3rdparty/libprocess/src/memory_profiler.cpp
Lines 387 (patched)
<https://reviews.apache.org/r/63368/#comment275958>

    This looks like a general pattern we use in libprocess. Can we add an overload for `route()`
and fix other places? Somehting like this:
    ```
    template <typename T>
    void route(
        const std::string& name,
        const Option<std::string>& realm,
        const Option<std::string>& help,
        Future<http::Response> (T::*method)(
            const http::Request&,
            const Option<http::authentication::Principal>&),
        const RouteOptions& options = RouteOptions())
    {
      if (realm.isSome()) {
        route(name, realm.get(), help, handler, options);
      } else {
        // Note that we use dynamic_cast here so a process can use
        // multiple inheritance if it sees so fit (e.g., to implement
        // multiple callback interfaces).
        HttpRequestHandler handler =
          lambda::bind(method, dynamic_cast<T*>(this), lambda::_1, None());
        route(name, help, handler, options);
      }
    }
    ```



3rdparty/libprocess/src/memory_profiler.cpp
Lines 395-398 (patched)
<https://reviews.apache.org/r/63368/#comment275957>

    I understand that you try to save some characters, but I'd argue and explicit `HELP` declaration
is superior. Moreover, it is consistent to the rest of the codebase. Please define `static
const std::string *_HELP();` functions



3rdparty/libprocess/src/memory_profiler.cpp
Lines 686-690 (patched)
<https://reviews.apache.org/r/63368/#comment275981>

    This looks to me that a situation when `rawId` is reset but the old file exists is possible.
It is probably fine for now, but deserves a comment.



3rdparty/libprocess/src/memory_profiler.cpp
Lines 714 (patched)
<https://reviews.apache.org/r/63368/#comment275982>

    Don't you need to check `graphId.isSome()`?



3rdparty/libprocess/src/memory_profiler.cpp
Lines 807 (patched)
<https://reviews.apache.org/r/63368/#comment275983>

    Do you check that the file actually exists before streaming it to the user? I don't see
it neither here, nor in `generateGraph()`. You probably want to put such check into `generateGraph()`
and reset `graphId` in case file is gone or can't be accessed.


- Alexander Rukletsov


On Oct. 27, 2017, 7:04 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2017, 7:04 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 ba9e78148932304ce1961b88e5f97180abd35586 
>   3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 8b8323f5fb4617aa9c22bbdb56af3d1cb0b06ea3

>   3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad 
> 
> 
> Diff: https://reviews.apache.org/r/63368/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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