mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 36106: cgroups: added cpuacct subsystem
Date Fri, 03 Jul 2015 19:47:44 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.

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 had 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 :)


- Ben


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