-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stdlib: Improve gem5 PyStats #996
base: develop
Are you sure you want to change the base?
stdlib: Improve gem5 PyStats #996
Conversation
a9665f0
to
d842e35
Compare
fd4a537
to
261c501
Compare
ee2d4f0
to
7fa7584
Compare
e893be9
to
a1c6b37
Compare
7dd21a7
to
515dac4
Compare
515dac4
to
350ef31
Compare
This isn't a true Base class, it's just a Vector. In gem5 all Vectors are Scalar Vectors. This change simplfies the naming. Change-Id: Ib8881d854ab18de6acbf0fb200db2de6a43621a7
The big thing missing from the Vector stats was that each position in the vector could have it's own unique id (a str, float, or int) and each position in the vector can have its own description. Therefore, to add this the Vector is represented as a dictionary mapping the unique ID to a Pystat Scaler (whcih can have it's own unique description. Change-Id: I3a8634f43298f6491300cf5a4f9d25dee8101808
As Distribution inherits from Vector, it should be constructed with a Dictionary of scalars (in our implementation, a dictionary mapping the vector position's unique id for each bin and the value of that bin). Change-Id: Ie603c248e5db4b6dd7f71cc453eebd78793f69a3
This appears to have no equivalent type in the CPP stats and was never utilized in PyStats. Change-Id: Ia9afc83b4159eb1ab2c6f44ec0ad86cd73f2a4f8
Change-Id: Icb2f691abf88ef4bac8d277e421329edb000209b
This was not used and easily confused with the other 'Vector' in PyStats. Change-Id: I9294bb0ae04db0537c87a5f50ce023fc83d587b8
This is inclusive of tests to ensure they have implemented correctly. Change-Id: I5c84d5ffdb7b914936cfd86ca012a7b141eeaf42
Change-Id: Iba0c93ffa5c4b18acf75af82965c63a8881df189
1. Thests here for the Scalar tasks are named appropriately. Not just generic "SimStats tess". 2. We remove 'simstat' terminology. The correct word is "Pystats". Change-Id: Idebc4e750f4be7f140ad6bff9c6772f580a24861
Change-Id: I1d93453072d12aa2dd40066f364723de1225b4e0
Change-Id: Iae12a0ac88f9646acb00e73d70f83b1e2ff94ac9
350ef31
to
0f6bd24
Compare
I believe I've fixed the failing tests. This should be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small things, but overall it looks great! Thanks for the hard work!!
Get all the stats in the vector of that name. Useful for performing | ||
operations on all the stats of the same name in a vector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an example of this? Reading the code and looking at the description, I don't know how I would use this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(jotting this down while it's fresh in my mind --- I will add a test later to verify it works like I think).
I can't really remember, but i think the reason for this function is for ases you have a simobject tree which makes stats look like this:
processor = {
cpus = [
{ stats1 = { stat2 = 5.6 } },
{ stats_a = { stat_a = { stat2 = 4.3 } } },
{ stat2 = 3.6 }
]
}
With your output to stats.txt looking something like:
processor.cpu0.stats1.stat2 = 5.2
processor.cpu1.stats_a.stats1.stat2 = 4.3
processor.cpu2.stat2 = 3.6
The argument someone had (I think) was the JSON output is worse if you just want to get all the stat2
stats from cores in a the processor
simobject. For these big tree like structures, there was a desire to have a way to just iterate through all the child nodes recursively and get all the stats with the name a specific name. Therefore, this function was created and would return {stat2 =5.2, stat2=4.3, stat2 = 3.6}
if you run something like processor.get_all_stats_of_name("stat2")
.
I think the typicall usecase here would be you've got a multicore system and you want to get an average of all the stats called X
across all cores.
This function also works for the case where you want just the stats1.stat2
stats. So, in our example, just {stats1.stat2 = 5.2, stats1.stat2=4.3}
. TL;DR: it's a way of getting all the stats that share a particular name basically.
I don't know if that's in anyway useful but I'm pretty sure that's why I added this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I think it makes sense to include. Can you add a code snippet example to the docstring?
This commit addresses Jason's comment (gem5#996 (comment)) which highlighted putting the `_m5.stats.processDumpQueue` call in the iteration through the `root` object in `get_simstat` caused this function be potentially called many times when it only needs to be called once. This chance moved this call to just before this iteration and will tehrefore only be called once (if required) per `get_simstat` execution. Change-Id: I16908b6dee063a0df7877a19e215883963bfb081
creation_time = datetime.now() | ||
time_converstion = ( | ||
None # TODO https://gem5.atlassian.net/browse/GEM5-846 | ||
) | ||
final_tick = Root.getInstance().resolveStat("finalTick").value | ||
sim_ticks = Root.getInstance().resolveStat("simTicks").value | ||
simulated_begin_time = int(final_tick - sim_ticks) | ||
simulated_end_time = int(final_tick) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of this code can go outside the for loop as well.
Get all the stats in the vector of that name. Useful for performing | ||
operations on all the stats of the same name in a vector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I think it makes sense to include. Can you add a code snippet example to the docstring?
This PR incorporates numerous improvements and fixes to the gem5 PyStats. This includes:
simobjectvec[0]
. (This replaces theVector
group PyStat)SparseHist
PyStats.Vector2d
to PyStats.Distribution
PyStats is fixed to be a vector of Scalars.