stats: organize and clarify resettable stats

Information
Submitter:	Lena Olson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11230:374d79447577
---------------------------
stats: organize and clarify resettable stats

Some stats (e.g. sim_insts) were traditionally not reset, because they were
intended for simulator-level measurement. However, some other stats (e.g.
sim_ticks) WERE reset. In addition, some derived stats used both reset and
unreset stats in the calculation, resulting in nonsense values. This patch
creates two sets of overall stats: one set that respects reset, and is useful
for measuring the simulated program, and one set that does not reset, useful for
studying the simulator itself.  The non-resettable stats are prefixed with
"simulator_", as they behave different from other stats.

Testing Done

Issue Summary

Description	From	Last Updated	Status
We should probably check with Andreas Sandberg on this one. It looks like ctrInsts is already a cummulative stat for ...	Joel Hestness	Jan. 11, 2016, 3:52 p.m.	Open
I'm hoping that we could use stat names that are a little more self-evident for these. Maybe instead of 'simulatorXxxx'/'simulator_xxxx' ...	Joel Hestness	Jan. 11, 2016, 3:52 p.m.	Open

Traditionally, it's not reset, as it accounting for all instructions executed. Perhaps hostSeconds shouldn't be reset as well and then the calculation would be correct? sim_insts isn't supposed to be used for anything other than simulator level measurement. If you want instructions executed on a cpu you should look at cpu.numInsts.

Lena Olson Nov. 25, 2015, 6:21 p.m. (Nov. 25, 2015, 6:21 p.m.)

Thanks for the response, Andreas -- I had wondered whether or not they were intentionally not reset, but even when I asked around no one seemed to know, so I think it's not as obvious as it could be. I would not be super surprised if someone tried to use it instead of adding up per-core commitedInsts, when calculating e.g. IPC. So I think in this case, I'll go through and make hostSeconds and similar stats unresetable as well, and if there are no objections, I'll add a note in the description for all non-resetable stats (similar to the final_tick description, except that it's from beginning of simulation, not restored from checkpoint).

Lena Olson Nov. 25, 2015, 6:32 p.m. (Nov. 25, 2015, 6:32 p.m.)

Actually, I have a question about this.  For sim_seconds and sim_ticks, my understanding is that these currently ARE reset.  Is that considered correct behavior?  I know these changes are a little nitpicky, but I think there is benefit to being consistent here.  Is there a way we can get all the sim_* stats to have the same behavior, and if so, is there a traditional correct behavior?

Joel Hestness Nov. 27, 2015, 10:39 a.m. (Nov. 27, 2015, 10:39 a.m.)

I really like that host_seconds resets. This allows the user to look at a set of stats dumps and decide how long it might take to simulate subsets of work (e.g. for simpoints or modifying a region of interest). Similarly, sim_ticks is nice to have reset, while final_tick (cumulative) is nice for estimating where to switch CPUs or where to start debug traces.

Since there are so few stats in question, it wouldn't really hurt to add new stats. I see value in having both cumulative and non-cumulative versions of instructions, ops, and simulation time. Would it be acceptable to just add the ones that don't currently exist?

Ali Saidi Nov. 28, 2015, 3:02 p.m. (Nov. 28, 2015, 3:02 p.m.)

Yea mean having both non-resetting and resetting versions of all the sim/host/... stats? Seems fine to me.

Change Summary:

Here's an attempt at duplicating and splitting up the stats. I had trouble deciding what to call things or how to make it clear which stats were which, so if anyone has better ideas of how to do that, I'd be happy to hear them.

Summary:

-	stats: make sim_insts and sim_ops respect stats reset
+	stats: organize and clarify resettable stats

Description:

~		Changeset 11229:f4b15e5109f9
	~	Changeset 11230:374d79447577

~		stats: make sim_insts and sim_ops respect stats reset
	~	stats: organize and clarify resettable stats

~		Because sim_insts and sim_ops were being calculated using the ThreadState
~		variable numInst/numOp (type Counter) rather than numInsts/numOps (type
~		Stats::Scalar), they were not getting reset. This behavior is confusing because
~		almost all other entries in the stats file do get reset (with the exception of
~		final_tick, which notes it is never reset in the stats file). It also leads to
~		incorrect behavior with stats like host_inst_rate, which reset the host time but
~		not the instructions executed. This patch resets sim_insts and sim_ops.
	~	Some stats (e.g. sim_insts) were traditionally not reset, because they were
	~	intended for simulator-level measurement. However, some other stats (e.g.
	~	sim_ticks) WERE reset. In addition, some derived stats used both reset and
	~	unreset stats in the calculation, resulting in nonsense values. This patch
	~	creates two sets of overall stats: one set that respects reset, and is useful
	~	for measuring the simulated program, and one set that does not reset, useful for
	~	studying the simulator itself. The non-resettable stats are prefixed with
	+	"simulator_", as they behave different from other stats.

Diff:

Revision 2 (+238 -12)

Show changes

	src/arch/null/cpu_dummy.hh
	src/cpu/BaseCPU.py
	src/cpu/base.hh
	src/cpu/checker/cpu.hh
	src/cpu/kvm/base.hh
	src/cpu/kvm/base.cc
	src/cpu/minor/cpu.cc
	src/cpu/o3/cpu.hh
	5 more

The implementation here looks good. Thanks for putting this together!

Just a couple small requests below.

src/cpu/kvm/base.cc (Diff revision 2)

We should probably check with Andreas Sandberg on this one. It looks like ctrInsts is already a cummulative stat for the KVM CPU. This might be required by the KVM CPU for fast-forwarding correctness? This could cause changes in KVM correctness and/or regressions (if any are maintained). Hopefully, it is harmless to allow the stat to reset, though?

Show all issues

Lena Olson Jan. 12, 2016, 11:10 a.m. (Jan. 12, 2016, 11:10 a.m.)

I think it should be okay -- ctrInsts and numInsts both already existed and I don't modify them, I just changed which is used by totalInsts(), which looks like it never gets called by anything besides the stats stuff. Another pair of eyes wouldn't hurt though.

Alexandru Dutu Jan. 21, 2016, 10:06 a.m. (Jan. 21, 2016, 10:06 a.m.)

This seems fine to me both ctrInsts and numInsts are incremented with the value from hwInstructions performance counter.

src/cpu/minor/cpu.cc (Diff revision 2)

Just a note that this is interesting... looks like this fixes an inconsistency: MinorCPU appears to collect inst stats cummulatively, while the other CPUs use resettable stats (another indicator that this patch will help with clarity!)

Lena Olson Jan. 12, 2016, 11:10 a.m. (Jan. 12, 2016, 11:10 a.m.)

I think all of them used numInst (which is a counter) originally, and I changed them all to numInsts (which is a stat)?  It's easy to get numInst and numInsts confused -- there being a counter called numInst and a stat called numInsts as members of the same struct is just asking for trouble IMO, but I didn't have a better idea so I left them as is.

src/sim/stat_control.cc (Diff revision 2)

I'm hoping that we could use stat names that are a little more self-evident for these. Maybe instead of 'simulatorXxxx'/'simulator_xxxx' we could use something like 'cummulativeSimXxxx'/'cumm_sim_xxxx'? That way the names more clearly indicate that the stats are accumulated throughout the whole simulation time rather than just the current stats period.

I do like that the descriptions say "not reset" also.

Show all issues

Lena Olson Jan. 12, 2016, 11:10 a.m. (Jan. 12, 2016, 11:10 a.m.)

I like cumulativeSim (and cumulative_sim, though it's long), as long as it's clear that it's different from final_tick -- it's cumulative over this invocation of the simulator, but not restored from checkpoints. Hopefully the note in the descriptions helps clarify that. Anyone else have opinions?

I'm glad you've taken the time to figure out what all these stats actually do. I like the direction, but a couple of comments.

First, can you add more comments to the code. Similar to what's in o3/cpu.hh. It would be good if this was at least in all of the .hh files, maybe in the .cc files too. Related to my next comment, but if the names aren't obvious, they should at least be very well commented.

Second, these names make no sense to me. Why are "sim*" reset and "simulator*" not reset? Isn't "sim" short for "simulator" or "simulation"?

My suggestion: For statitics that are reset every time, have no prefix at all so there would be ticks, instructions, etc. We already know the came from a simulator/simulation.
For the statistics that don't reset you could call them "total*" (i.e., totalTicks, totalInsts).
And I think finalTicks is fine as it is with its extra comments.

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Issue Summary

Change Summary:

Summary:

Description:

Changeset 11229:f4b15e5109f9

Changeset 11230:374d79447577

Diff: