----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/704/#review1237 -----------------------------------------------------------
Overall, this is a good start. There are a couple of recurring themes in my review. 1) Objects should have a name() function (this is automatic for SimObjects) and we should use that to generate the stat name simply by doing something like this: ".name(name() + ".my_stat"). 2) Stats should be opaque and you shouldn't try to grab the value of them. If you need the value, you should create another variable. This is because stats values can be arbitrarily screwed with by the user. 3) Stats shouldn't be within a std::vector and they shouldn't be dynamically allocated. We should figure out how to use arrays or the various Vector stats to do what we need. 4) printStats and clearStats functions should go away. src/mem/ruby/network/simple/Throttle.hh <http://reviews.m5sim.org/r/704/#comment1696> I know that this is old code, but this code will leak memory. Can you plan to fix it? src/mem/ruby/network/simple/Throttle.hh <http://reviews.m5sim.org/r/704/#comment1697> This really shouldn't work this way. You should use Stat::Vector2d. src/mem/ruby/network/simple/Throttle.cc <http://reviews.m5sim.org/r/704/#comment1698> Please sort your #includes. The style hook should have complained about (and offered to fix) this. Do you not have it installed correctly. Also, shouldn't this just be "base/statistics.hh" src/mem/ruby/network/simple/Throttle.cc <http://reviews.m5sim.org/r/704/#comment1699> Stats really should not be dynamically allocated. src/mem/ruby/network/simple/Throttle.cc <http://reviews.m5sim.org/r/704/#comment1703> can I suggest csprintf() instead of stringstream? Also, SimObjects generally have a name() parameter and we generally name stats like this: my_stat .name(name() + ".my_stat") .desc(...) src/mem/ruby/network/simple/Throttle.cc <http://reviews.m5sim.org/r/704/#comment1700> can I suggest csprintf() instead of stringstream? src/mem/ruby/network/simple/Throttle.cc <http://reviews.m5sim.org/r/704/#comment1701> It's not necessary to initialize stats to zero. This is automatic. src/mem/ruby/network/simple/Throttle.cc <http://reviews.m5sim.org/r/704/#comment1702> Just to check, is this function actually used? Because stats can be reset at any time and updated in strange ways, we treat stats as opaque objects and never try to extract the current value. If link utilization is important, we should have a separate variable for this. src/mem/ruby/profiler/AddressProfiler.cc <http://reviews.m5sim.org/r/704/#comment1704> .name should really have name() as part of the name. Is the AddressProfiler not a SimObject() ? src/mem/ruby/profiler/AddressProfiler.cc <http://reviews.m5sim.org/r/704/#comment1706> Probably should be removed. src/mem/ruby/profiler/AddressProfiler.cc <http://reviews.m5sim.org/r/704/#comment1705> The statistics package has its own mechanism for clearing statistics. I think that you should hook into that and not call clearStats yourself. src/mem/ruby/profiler/CacheProfiler.hh <http://reviews.m5sim.org/r/704/#comment1707> Does a Stats::Vector make more sense here? src/mem/ruby/profiler/CacheProfiler.cc <http://reviews.m5sim.org/r/704/#comment1708> We should really figure out how to have the profilers have a name() parameter. If we do it this way, we'll never be able to instantiate two separate systems that have a ruby memory hierarchy. src/mem/ruby/profiler/CacheProfiler.cc <http://reviews.m5sim.org/r/704/#comment1709> Shouldn't this whole function go away? src/mem/ruby/profiler/MemCntrlProfiler.cc <http://reviews.m5sim.org/r/704/#comment1710> Need to use name() src/mem/ruby/profiler/MemCntrlProfiler.cc <http://reviews.m5sim.org/r/704/#comment1711> This should go away. src/mem/ruby/profiler/Profiler.hh <http://reviews.m5sim.org/r/704/#comment1712> regStats() is the proper name. src/mem/ruby/profiler/Profiler.hh <http://reviews.m5sim.org/r/704/#comment1713> Again, we really shouldn't dynamically allocate statistics. What exactly is this stat all about? src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1714> Please delete code instead of commenting it out. In fact, this whole function should probably be deleted. src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1718> How did you pick this magic number? Can we at least make it a constant that is defined in a header file with an explanation as to how it was chosen? src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1719> More magic numbers. src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1715> This should probably be deleted. src/mem/ruby/system/CacheMemory.hh <http://reviews.m5sim.org/r/704/#comment1716> If the profiler is a SimObject, regStats will automatically get called. src/mem/ruby/system/Sequencer.hh <http://reviews.m5sim.org/r/704/#comment1717> Hmmm. More vectors of histograms. Can we make this an array instead? src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/704/#comment1720> more magic numbers. src/mem/slicc/symbols/StateMachine.py <http://reviews.m5sim.org/r/704/#comment1721> can we use name() ? src/mem/slicc/symbols/StateMachine.py <http://reviews.m5sim.org/r/704/#comment1722> Can we avoid the dynamic allocation here? src/mem/slicc/symbols/StateMachine.py <http://reviews.m5sim.org/r/704/#comment1723> Is this used outside of stats? src/mem/slicc/symbols/StateMachine.py <http://reviews.m5sim.org/r/704/#comment1724> is this used outside of stats? - Nathan On 2011-05-16 15:06:16, Derek Hower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/704/ > ----------------------------------------------------------- > > (Updated 2011-05-16 15:06:16) > > > Review request for Default, Nathan Binkert, Korey Sewell, and Brad Beckmann. > > > Summary > ------- > > This patch contains changes to convert Ruby's stat handling to the M5-style > Stat class. The ultimate goal is to remove Profiler entirely, though this > patch only represents a (significant) step towards that goal. Some stats > remain in the Profiler, notably those that do not have an obvious object to > hold them (like Address Profiler) or those that I'm not sure what they do > (e.g., *wCC*). Also, this patch does not contain a Garnet stats conversion > (though the simple network is converted). > > > Diffs > ----- > > src/mem/ruby/network/simple/SimpleNetwork.hh UNKNOWN > src/mem/ruby/network/simple/SimpleNetwork.cc UNKNOWN > src/mem/ruby/network/simple/Throttle.hh UNKNOWN > src/mem/ruby/network/simple/Throttle.cc UNKNOWN > src/mem/ruby/profiler/AddressProfiler.hh UNKNOWN > src/mem/ruby/profiler/AddressProfiler.cc UNKNOWN > src/mem/ruby/profiler/CacheProfiler.hh UNKNOWN > src/mem/ruby/profiler/CacheProfiler.cc UNKNOWN > src/mem/ruby/profiler/MemCntrlProfiler.hh UNKNOWN > src/mem/ruby/profiler/MemCntrlProfiler.cc UNKNOWN > src/mem/ruby/profiler/Profiler.hh UNKNOWN > src/mem/ruby/profiler/Profiler.cc UNKNOWN > src/mem/ruby/slicc_interface/RubySlicc_Profiler_interface.hh UNKNOWN > src/mem/ruby/slicc_interface/RubySlicc_Profiler_interface.cc UNKNOWN > src/mem/ruby/system/CacheMemory.hh UNKNOWN > src/mem/ruby/system/MemoryControl.hh UNKNOWN > src/mem/ruby/system/Sequencer.hh UNKNOWN > src/mem/ruby/system/Sequencer.cc UNKNOWN > src/mem/ruby/system/System.cc UNKNOWN > src/mem/slicc/symbols/StateMachine.py UNKNOWN > > Diff: http://reviews.m5sim.org/r/704/diff > > > Testing > ------- > > > Thanks, > > Derek > > _______________________________________________ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
