-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-119692: Add Total UOp 'cost' to PyStats Output #119693
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
base: main
Are you sure you want to change the base?
gh-119692: Add Total UOp 'cost' to PyStats Output #119693
Conversation
mdboom
left a comment
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.
Thanks. This LGTM, but let's let @brandtbucher confirm the stats themselves make sense for what he needs.
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.
Thanks for doing this. I think it will be really useful!
A few notes:
- I don't love searching for and parsing
jit_stencils.hlike this... it's pretty fragile (the JIT is changing quite a bit right now, and out-of-tree builds are a thing unfortunately). A more robust solution would be to dump the code size as part of the stats themselves in the interpreter, since we have access to thestencil_groupsarray in the C code. Then we could just parse it out of the stats dump normally. - No need to repeat the "Self" and "Cumulative" values in the new table.
- I'd rename "length" to "size" (minor, but we use "length" to mean other things in the stats summary).
- Remove all mention of "machine instructions", since what we're really measuring is size (in bytes).
- I'd replace "Product" with something like "Total Size" or "Total Cost" in the table, and move it after the "Count" and "Size" columns (which makes it a bit clearer to me that it is derived from them).
|
Thanks for the feedback @brandtbucher! I've reworked things so that the code size (and data size) of each stencil are dumped as part of the stats, and the summarize script picks them up from there. I've also renamed and re-ordered the table fields to clarify what is being shown. I've left the The new table with some sample data looks like: Total Bytes Executed per JIT'ed UOp
** Updated - see below ** # Data about JIT stencils isn't cumulative
if "code_size" in key or "data_size" in key:
stats[key.strip()] = int(value)
else:
stats[key.strip()] += int(value)
|
|
I made a small format change - keys that have I think ideally, there would be some checking that these values are consistent across all the stats files, but currently it'll just use the last value it finds. A bit of a kludge still, but since I would guess it's rare to have stats files hanging around from multiple builds with different jit stencils, perhaps this is fine for now? |
This PR adds an additional table to the output from
summarize_stats.py. Namely, a table of(# of times a uop was exectued) * (length of that UOp in machine code), sorted by this value. This makes it clear how much time* is being spent in each UOp, as opposed to just which ones are most frequently executed.*Machine instruction count is a rough proxy for time, but a really easy one to calculate.
The new table looks like this:
Total Machine Instruction Counts per UOp
Closes #119692. Tagging @brandtbucher as the requester of this feature, and @mdboom for pystats visibility.