mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 36106: cgroups: added cpuacct subsystem
Date Sun, 05 Jul 2015 15:57:56 GMT


> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > <https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014>
> >
> >     Any reason you're not just re-using cgroups::stat here? I'd suggest just calling
cgroups::stat for now, should simplify this and make it easier for the reader. :)
> 
> Jojy Varghese wrote:
>     The only reason being that the way cpuacct creates Stat should be encapsulated in
the cpuacct::Stat. This is the same reason there is a parse method in Stat. But I can change
it to use cgroups::stat if absolutely necessary.
> 
> Jojy Varghese wrote:
>     Also wanted to highlight the semantics of Stat class to make it clear what the intent
is. Stat can only be created from data read from the stats file (cpuacct.stat). Thus creating
Stat without the file data should not be allowed. Once the Stat object is created, you can
not mutate it.
> 
> Ben Mahler wrote:
>     Imagine this only returned the user time, then we would return a Duration, yes? We
would not be returning a `Stat` class with one const 'user' member, with a const getter and
a static parse method. That would be overkill, right?
>     
>     That's the reasoning I have for making this a plain struct. This is the same situation
as above, but we need to return more than one value (e.g. `os::UTSInfo`, `os::Memory`, `os::Load`,
etc.)
> 
> Jojy Varghese wrote:
>     If we were to just return a single value, I would have preferred it to be a rvalue.
Duration is a possiblity but not ideal in this situation as we need just the ticks to seconds
convertion which is not provided by Duration. Structures like UTSInfo, Memory etc should be
non-mutable as they represet a snapshot of data IMHO.
> 
> Ben Mahler wrote:
>     Why would the caller care about the ticks conversion? The caller just wants to know
the amount of cpu time (i.e. duration) spent in userland, the tick conversion is an implementation
detail of reading the file format, which the caller does not want to be burdened with.
>     
>     I didn't understand your comment about returning an rvalue, the return value at the
callsite is an rvalue inherently until you assign it a name.. and the return value in the
function could be an rvalue if the Duration is constructed inline at the end.. something I'm
missing here? Assuming that you're referring to the function body, why would you care about
returning an rvalue when we have copy elision?
>     
>     Back to the point, from the ideological perspective, I agree with you, immutability
can be a very nice functional programming principle. However, the perspective I'm coming from
is the experiential perspective, one of having worked on and maintained this code base for
a few years, and having a keen interest in seeing things remain simple, straightforward, and
maintainable. I'd like all of us to be able to come in to any piece of code and be able to
read and understand it without unnecessary effort. What I've seen from when we've tried to
embrace immutable structs is that the hit to simplicity and straightforwardness is too high.
First, one adds 'const' members, which requires a constructor. Then, we run into the difficulty
of having no default assignment and/or assignment operator. So, things are made non-const,
and to keep const semantics, one has to add const getters to ensure the caller cannot modify
the members. Now this is usually ~5x the amount of code as we originally ha
 d with a simple struct. The cognitive burden of understanding the immutable 'Stat' abstraction
is higher because it is inherently more complicated and requires more time to read:
>     
>     ```
>     struct Stat
>     {
>       Duration user;
>       Duration system;
>     }
>     
>     // VS
>     
>     class Stat
>     {
>     public:
>       // Sometimes this is private and there is a factory. 
>       Stat(Duration user, Duration system)
>         : user_(user), system_(system) {}
>       
>       // Default construction for use as values in STL maps, etc.
>       Stat() {}
>       
>       Duration user() const
>       {
>         return user_;
>       }
>     
>       Duration system() const
>       {
>         return system_;
>       }
>     
>     private:
>       // Non-const for assignability / default construction.
>       Duration user_;
>       Duration system_;  
>     }
>     ```
>     
>     So while there are indeed situations where immutability is nice (e.g. state::Variable),
the cognitive overhead the code reader faces with the more complicated Stat class is not worth
it, when we're dealing with simple wrappers of data, IMHO :)

Duration would be an overkill when "userTimeInSecs" is what is the only thing required. But
i dont have any strong opininions about it. But when we say that a good design principle is
an overkill, then that is a grey area. Readbility is a function of experience. Having strong
design principles in the code is an indication of robustness and thoughtfulness. Using the
language to fully express the intent should be encouraged.
  Having said that, i dont think I can get this passed the review. So i will change the code
as per your suggestion :)


- Jojy


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


On July 2, 2015, 8:09 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> -----------------------------------------------------------
> 
> (Updated July 2, 2015, 8:09 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> cgroups implementation does not have a cpuacct subsystem implementation as of
> today. Adding the implementation for stat function.
> 
> Changes:
>   - added Stats class to encapsulate cpuacct.stat data
>   - added implementation for cpuacct::stats
>   - added unit tests
> 
> Jira: MESOS-2961
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
>   src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
>   src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 
> 
> Diff: https://reviews.apache.org/r/36106/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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