-
Notifications
You must be signed in to change notification settings - Fork 65
Update jit-analyze to handle multiple metrics #223
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
Update jit-analyze to handle multiple metrics #223
Conversation
Alternative take to dotnet#215 where jit-analyze is updated to look for and process multiple bits of metric data. With the pattern shown here it should be simple to add additional metrics like the ones we already have.
|
cc @dotnet/jit-contrib Might be a bit slower, but I think processing time is dominated by file IO and text handling. Doesn't fully handle the "higher is better" case yet, but we don't have any metrics like that yet. Sample output for perf score: |
BruceForstall
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.
LGTM!
|
@briansull This should make your change #215 unnecessary. |
|
I believe that there are still parts on my chnage in #215 that are still needed.
|
briansull
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.
Thank you Andy!
| } | ||
|
|
||
| public class PrologSizeMetric : Metric | ||
| { |
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'm not sure if we care about PrologSizes, in my change I just removed it.
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.
Seems harmless to keep it, though. I expect going forward we will have lots of metrics.
|
I can add your jit-diffs changes, though will probably let count default to the jit-analyze default (which is 5), and will use --metric instead of hard-coded metric options. |
OK that would be fine with me. Although I find that 5 is too small. |
|
We'll give 20 a try. Fixed a few small issues in jit-analyze that I overlooked, including a build break. Not sure what the CI does here but it evidently doesn't try to build...? |
CarolEidt
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.
Overall, LGTM. I might have simply omitted the boolean for higher-is-better vs lower, since it's not actually supported (or currently needed) and then added it as needed, but I don't feel strongly about it.
|
@AndyAyersMS The CI does build, but it apparently can't tell when a build succeeds or not (you can see your failure in the build logs: https://dev.azure.com/dnceng/public/_build/results?buildId=416893) cc @echesakovMSFT |
|
Ah, probably just using the repo build scripts, which don't propagate errors. |
|
Just a reminder on the new |
Alternative take to #215 where jit-analyze is updated to look for
and process multiple bits of metric data.
With the pattern shown here it should be simple to add additional
metrics like the ones we already have.