Skip to content
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

Add support for storing method handle histograms in profiles #67919

Merged
merged 8 commits into from
May 3, 2022

Conversation

jakobbotsch
Copy link
Member

Allow method handle histograms in .mibc files and in the PGO text format.

Contributes to #44610.

@davidwrighton would this in theory need a R2R version bump? I suppose we can wait with such a bump until we actually start producing profiles with this data (at which point we should also add an equivalent of GetLikelyClass)?

cc @dotnet/jit-contrib

Allow method handle histograms in .mibc files and in the PGO text
format.

Contributes to dotnet#44610.
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jit and SPMI changes look good.

WIll defer to David or Michal for the remainder of the changes.

@BruceForstall
Copy link
Member

Do you need to change the JIT-EE GUID?

@jakobbotsch
Copy link
Member Author

Do you need to change the JIT-EE GUID?

I have some follow-up changes that will actually start producing and using the new kind of PGO data. I think we can delay updating the JIT-EE GUID to that as just adding new PGO schema kinds should be binary compatible (and my future changes will probably also need another JIT-EE GUID update).

@davidwrighton
Copy link
Member

@jakobbotsch Yes, this requires a bump in the R2R minor version.

@davidwrighton
Copy link
Member

@jakobbotsch Have you verified that with these changes the dotnet-pgo tool continues to work with .NET 6 runtimes?

@jakobbotsch
Copy link
Member Author

@jakobbotsch Yes, this requires a bump in the R2R minor version.

Is it ok to wait until future changes that actually start producing and consuming these? At that point I will add something similar to GetLikelyClass for methods which will require another R2R version bump.

@jakobbotsch Have you verified that with these changes the dotnet-pgo tool continues to work with .NET 6 runtimes?

Yes, it produced a .mibc with the same hash as dotnet-pgo from current main in my test.

@davidwrighton
Copy link
Member

When in doubt, bump the version. If there is no chance that new data could be produced, you don't need to, but bumping the R2R version is pretty cheap, and doesn't come with significant costs as old R2R code should not be broken.

@jakobbotsch
Copy link
Member Author

@davidwrighton I've bumped the R2R minor version and also the JIT-EE GUID to be safe, please take another look.
I plan to wait with merging this until after the snap in any case.

@jakobbotsch
Copy link
Member Author

ping @davidwrighton, anything else you think I should do here?

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked at JIT / SPMI in depth.

@jakobbotsch
Copy link
Member Author

Ping @davidwrighton, my follow-up depends on this one.

@jakobbotsch jakobbotsch merged commit 6f563b3 into dotnet:main May 3, 2022
@jakobbotsch jakobbotsch deleted the method-handle-histograms branch May 3, 2022 17:19
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants