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