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

Provide block counts in tiered compilation from R2R images #13672

Closed
davidwrighton opened this issue Oct 28, 2019 · 9 comments · Fixed by #70941
Closed

Provide block counts in tiered compilation from R2R images #13672

davidwrighton opened this issue Oct 28, 2019 · 9 comments · Fixed by #70941
Labels
Milestone

Comments

@davidwrighton
Copy link
Member

Tiered compilation can reduce the performance of application by replacing IBC tuned R2R code with non-tuned jitted code. See #13451 Consider enhancing the R2R format to allow method block counts to be stored in R2R images and used by the runtime. This is causing a ~20% regression in the performance of the StringBuilder.Append code

@EgorBo
Copy link
Member

EgorBo commented Oct 28, 2019

Not sure it's related but today I found this gem:
https://github.com/dotnet/coreclr/blob/8460516ceb23458f7a279bb73aa6176e17e8a90d/src/jit/emit.cpp#L4542-L4552

PS: Don't we need a kind of deoptimization in order to rely on PGO data? E.g. something in the app session changed and now blocks' weights are not relevant anymore.

@BruceForstall
Copy link
Member

cc @briansull @AndyAyersMS

@briansull
Copy link
Contributor

briansull commented Oct 28, 2019

PS: Don't we need a kind of deoptimization in order to rely on PGO data? E.g. something in the app session changed and now blocks' weights are not relevant anymore.

Yes, That is already what we do when a method is modified and the ILsize no longer matches.

https://github.com/dotnet/coreclr/blob/2f1d783cbaa806981c6640d85a28da8f5875b735/src/jit/compiler.cpp#L3516-L3533

@davidwrighton
Copy link
Member Author

I think that @EgorBo is referring to the idea that the application may have different typical pathways through code than the training data. While this is a serious concern, historically we've had this problem for the binaries we shipped to customers for many years on the desktop framework, and the performance impact was not a large problem for the scenarios where our optimizations have kicked in in unfortunate ways.

I believe (but do not have strong evidence for my belief) that this is due to the fairly limited sets of optimizations that take advantage of this profile data. I also believe this to be a higher risk if we start to take a heavier dependence on profile data for optimizations such as devirtualization. Since our training scenarios are quite limited in CoreCLR, I expect that when we implement this feature, we will have a good opportunity to observe potential positive/negative effects on our performance results and make a judgement of regressions at that time.

@sergiy-k
Copy link
Contributor

/cc: @kouvel

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@adamsitnik adamsitnik added the tenet-performance Performance related issue label Jul 8, 2020
@davidwrighton davidwrighton modified the milestones: 5.0.0, 6.0.0 Aug 10, 2020
@davidwrighton
Copy link
Member Author

This work won't make .NET 5.

@davidwrighton
Copy link
Member Author

EgorBo is working on this with #70941

@davidwrighton davidwrighton linked a pull request Jul 8, 2022 that will close this issue
@mangod9
Copy link
Member

mangod9 commented Aug 5, 2022

@EgorBo do we expect this to make it into 7?

@davidwrighton
Copy link
Member Author

We implemented this particular feature with --embed-pgo-data which we use only for System.Private.CoreLib. @EgorBo's work provides the ability to get live runtime based pgo data, which is even better.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants