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

Enable the latest managed pgo data #49793

Merged

Conversation

davidwrighton
Copy link
Member

  • Implementation is parallel to existing ibc handling, so that it can be toggled on/off by adjusting the UsingToolIbcOptimization property
  • Use the same data for all assemblies produced in current build
  • Apply data to release builds only
  • Disable mismatch assertions in jit for current state where il mismatches are common

@davidwrighton davidwrighton requested a review from trylek March 18, 2021 02:24
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@davidwrighton
Copy link
Member Author

@dotnet/crossgen-contrib
@agocke

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 changes ok (per offline discussion).

This data flows through to jitting (right?). If so we might want to look at what happens to jit codegen. I can do that separately though.

@mangod9
Copy link
Member

mangod9 commented Mar 18, 2021

Since IBC is currently not used/supported should that code be removed?

@Lxiamail Lxiamail self-requested a review March 18, 2021 05:00
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! Just for my curiosity, do the MIBC data restoration failures mean we first need to somehow seed the optimization data or are we going to need to make the restoration optional?

eng/restore/optimizationData.targets Outdated Show resolved Hide resolved
@@ -116,6 +116,9 @@
<SystemSecurityCryptographyX509CertificatesTestDataVersion>5.0.0-beta.21159.1</SystemSecurityCryptographyX509CertificatesTestDataVersion>
<SystemWindowsExtensionsTestDataVersion>5.0.0-beta.21159.1</SystemWindowsExtensionsTestDataVersion>
<!-- dotnet-optimization dependencies -->
<optimizationwindows_ntx64MIBCRuntimeVersion>99.99.99-master-20210317.2</optimizationwindows_ntx64MIBCRuntimeVersion>
<optimizationwindows_ntx86MIBCRuntimeVersion>99.99.99-master-20210317.2</optimizationwindows_ntx86MIBCRuntimeVersion>
<optimizationlinuxx64MIBCRuntimeVersion>99.99.99-master-20210317.2</optimizationlinuxx64MIBCRuntimeVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Is nuget package "optimization.MIBC.Runtime.99.99.99-master-20210317.2.nupkg" needed anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally the complex logic that is used to choose which one of these applies to a given platform will change to use the non-arch specific nuget package, but for now I just made it work without it.

eng/Versions.props Show resolved Hide resolved
@davidwrighton davidwrighton merged commit 5d4a851 into dotnet:main Mar 19, 2021
@AndyAyersMS
Copy link
Member

@DrewScoggins @adamsitnik

FYI, this PR may lead to widespread changes in benchmark perf as static PGO data will now be available to the jit for some assemblies. Hopefully, many many improvements...

@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2021
@davidwrighton davidwrighton deleted the Full_pgo_optimization_changes branch April 20, 2021 17:47
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
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.

6 participants