> On April 28, 2012, 9:08 a.m., Steve Reinhardt wrote: > > I like the idea of having a common way of collecting these stats (and > > probably more importantly, common names and semantics for the output). > > Does it make sense to have this in a separate object though? It seems like > > that just adds complexity to the configuration, and the possibility of not > > putting one of these where you wish you had. Did you consider putting all > > this code in a base class like MemObject so that everyone automatically > > collects all these stats?
A very valid comment, and this option was under serious consideration. The benefits of having it behaviourally encapsulated (as you now propose) rather than structurally encapsulated (as the current patch does) is indeed that: 1) it does not involve changing any configuration scripts, and 2) any communicating object would have a uniform set of stats relating to the communication The issues are: 1) how to "hide" it in the interface timing calls, as we must catch the case where sendTiming returns false and undo the changes to the senderState 2) the rather extensive set of disable flags in the object parameters 3) the potential negative performance impact caused by collecting more stats than we really want 4) more "noise" in the stats output 5) potential negative performance impact even when not collecting stats due to the large number of if-statements The current CommMonitor may not be ideal in that it requires config script changes, and it is indeed up to the user to determine which points in the memory system are of interest. However, I would expect no more than a handful monitors in a typical system, and the stats in the monitor could become multi-dimensional and indexed on the masterId in a future patch. As a minor side note, the structural CommMonitor also more closely resembles the bus monitors used in actual devices. Given all this, are you fine to proceed with this patch? Any other opinions or suggestions from the team? - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1159/#review2616 ----------------------------------------------------------- On April 26, 2012, 8:04 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1159/ > ----------------------------------------------------------- > > (Updated April 26, 2012, 8:04 a.m.) > > > Review request for Default. > > > Description > ------- > > MEM: Add the communication monitor > > This patch adds a communication monitor MemObject that can be inserted > between a master and slave port to provide a range of statistics about > the communication passing through it. The communication monitor is > non-invasive and does not change any properties or timing of the > packets, with the exception of adding a sender state to be able to > track latency. The statistics are only collected in timing mode (not > atomic) to avoid slowing down any fast forwarding. > > An example of the statistics captured by the monitor are: read/write > burst lengths, bandwidth, request-response latency, outstanding > transactions, inter transaction time, transaction count, and address > distribution. The monitor can be used in combination with periodic > resetting and dumping of stats (through schedStatEvent) to study the > behaviour over time. > > In future patches, a selection of convenience scripts will be added to > aid in visualising the statistics collected by the monitor. > > > Diffs > ----- > > src/mem/CommMonitor.py PRE-CREATION > src/mem/SConscript 6d11b01e2c53 > src/mem/comm_monitor.hh PRE-CREATION > src/mem/comm_monitor.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/1159/diff/ > > > Testing > ------- > > Used in combination with various full system simulations > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
