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 (Diff revision 1)

I know that this is old code, but this code will leak memory.  Can you plan to fix it?

1. Derek Hower May 17, 2011, 5:33 a.m. (May 17, 2011, 5:33 a.m.)

   yep.

src/mem/ruby/network/simple/Throttle.hh (Diff revision 1)

This really shouldn't work this way.  You should use Stat::Vector2d.

src/mem/ruby/network/simple/Throttle.cc (Diff revision 1)

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 (Diff revision 1)

Stats really should not be dynamically allocated.

src/mem/ruby/network/simple/Throttle.cc (Diff revision 1)

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 (Diff revision 1)

can I suggest csprintf() instead of stringstream?

src/mem/ruby/network/simple/Throttle.cc (Diff revision 1)

It's not necessary to initialize stats to zero.  This is automatic.

src/mem/ruby/network/simple/Throttle.cc (Diff revision 1)

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.

1. Derek Hower May 18, 2011, 3:15 a.m. (May 18, 2011, 3:15 a.m.)

   Yes it is used elsewhere. I'll mirror the stat with a standard variable

src/mem/ruby/profiler/AddressProfiler.cc (Diff revision 1)

.name should really have name() as part of the name.  Is the AddressProfiler not a SimObject() ?

1. Derek Hower May 18, 2011, 3:16 a.m. (May 18, 2011, 3:16 a.m.)

   I don't think it is.

2. Nathan Binkert May 18, 2011, 3:31 a.m. (May 18, 2011, 3:31 a.m.)

   Can I suggest that you create a name() function anyway and have some way to set the name variable?  This will facilitate conversion to a SimObject in the future and make things like DPRINTF work better (DPRINTF requires name())

src/mem/ruby/profiler/AddressProfiler.cc (Diff revision 1)

Probably should be removed.

src/mem/ruby/profiler/AddressProfiler.cc (Diff revision 1)

The statistics package has its own mechanism for clearing statistics.  I think that you should hook into that and not call clearStats yourself.

1. Derek Hower May 18, 2011, 3:16 a.m. (May 18, 2011, 3:16 a.m.)

   noted.

src/mem/ruby/profiler/CacheProfiler.hh (Diff revision 1)

Does a Stats::Vector make more sense here?

src/mem/ruby/profiler/CacheProfiler.cc (Diff revision 1)

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 (Diff revision 1)

Shouldn't this whole function go away?

src/mem/ruby/profiler/MemCntrlProfiler.cc (Diff revision 1)

Need to use name()

src/mem/ruby/profiler/MemCntrlProfiler.cc (Diff revision 1)

This should go away.

src/mem/ruby/profiler/Profiler.hh (Diff revision 1)

regStats() is the proper name.

src/mem/ruby/profiler/Profiler.hh (Diff revision 1)

Again, we really shouldn't dynamically allocate statistics.  What exactly is this stat all about?

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

Please delete code instead of commenting it out.  In fact, this whole function should probably be deleted.

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

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?

1. Derek Hower May 17, 2011, 5:31 a.m. (May 17, 2011, 5:31 a.m.)

   All magic numbers came from whatever Ruby was doing before the conversion (typically in the default initialization for Histograms). Making it a constant sounds fine.

2. Korey Sewell May 17, 2011, 12:13 p.m. (May 17, 2011, 12:13 p.m.)

   I can attest to these hardcoded values being the root of many (not all!) Ruby evils. 
   
   This is the histogram bucket size and the tricky thing is these also get reset with a clear(200) in the clearStats() function. The clearStats() functions should get whacked (nate's comments below), but ideally, make the histogram bucket size a parameter with a default value here.

3. Korey Sewell May 18, 2011, 4:27 a.m. (May 18, 2011, 4:27 a.m.)

   The histogram is initialized with size per bin and also number of bins. 
   
   I really don't think this should be a constant (this has caused me some headache before).
   
   Instead, a parameter with a default value of 200 sounds about right, no? Also, where does the bin size get initialized for the histogram stat? (Another candidate for a default parameter.)

4. Nathan Binkert May 18, 2011, 4:36 a.m. (May 18, 2011, 4:36 a.m.)

   Yeah, a parameter does make sense.  As for bin size, that's automatic.  Bin sizes start at 1 and automatically double until they can hold all samples.

5. Korey Sewell May 18, 2011, 4:45 a.m. (May 18, 2011, 4:45 a.m.)

   I agree that makes sense in some instances, but I've found that when you do that, it makes it hard to extract stats that can compare to runs of different systems. For example, when I'm comparing stats across different systems, I want to easily compare how many times a coherency miss fell within the 10-20 cycle range. When you don't have that, you have one run w/a stat range of 10-20 and another with 10-50. That gets messy ... quick. 
   
   That's why I think it's advantageous to define a bin size and then a saturation point. Maybe called that "saturating histogram" instead of the regular histogram.
   
   It was a easy fix for me to do in the Ruby tree, but if that's not something straightforward for gem5, just mark that down as a @todo or limitation of the histograms.

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

More magic numbers.

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

This should probably be deleted.

src/mem/ruby/system/CacheMemory.hh (Diff revision 1)

If the profiler is a SimObject, regStats will automatically get called.

src/mem/ruby/system/Sequencer.hh (Diff revision 1)

Hmmm.  More vectors of histograms.  Can we make this an array instead?

1. Korey Sewell May 17, 2011, 12:25 p.m. (May 17, 2011, 12:25 p.m.)

   These histograms dynamically size.
   
   Does the default in M5 do the same? and using the same resizing algorithm?

src/mem/ruby/system/Sequencer.cc (Diff revision 1)

more magic numbers.

src/mem/slicc/symbols/StateMachine.py (Diff revision 1)

can we use name() ?

src/mem/slicc/symbols/StateMachine.py (Diff revision 1)

Can we avoid the dynamic allocation here?

src/mem/slicc/symbols/StateMachine.py (Diff revision 1)

Is this used outside of stats?

src/mem/slicc/symbols/StateMachine.py (Diff revision 1)

is this used outside of stats?

I apologize it took me a while to review this code.  Overall, my major concern is that the patch removes some of the most valuable stats in Ruby.  Specific comments below:

src/mem/ruby/profiler/Profiler.hh (Diff revision 1)

Please, do not delete these stats.  These stats are very useful in discovering performance bottlenecks in protocols.  As far as those protocols checked into the tree, I believe the hammer protocol uses most, if not all of these stats.  The term "wCC" stands for "with Cache Coherence" and signifies those requests that require cache-to-cache transfers.

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

This code was used to monitor cache port and bank contention when Ruby modeled those resources.  I'm pretty sure that no current protocols enable the bank resource feature, but if the port resource is approximated by the number of transitions per cycle.  It is possible for that port resource to cause problems and this stat would catch it.  Thus I would like to keep these stats. 

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

Yes, Korey your last observation is exactly why those histograms are initialized with a fixed bin size.  It makes comparisons between different runs possible.  I assume you can do the same thing with M5 stats, correct?

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

Did you move this logic somewhere else, or did you determine that none of the checked in protocols use this function?

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

Please keep this funcitonality.  It is very useful to understand the behavior of the system.

1. Korey Sewell June 20, 2011, 5:14 a.m. (June 20, 2011, 5:14 a.m.)

   I'd like to ping on this. Is there anything I can do to help with this? It's really useful to not have to parse two files to obtain cpu and memory stats respectively. I can download a diff and see where we get but I dont want to redo work already done by the Wisconsin guys.
   
   Thoughts?

src/mem/ruby/profiler/Profiler.hh (Diff revision 1)

In my recent Ruby experiences, I agree with Brad to keep most if not all of these. Can we just convert these to M5 with the same names?

Subsequent changesets could consider if somethign is unnnecessary.

Also "missLatency" is really "accessLatency" as previously discussed. Can we change that?

src/mem/ruby/profiler/Profiler.cc (Diff revision 1)

We are going to call this access latency eventually right? Might as well change it now...