Skip to content

Use newest PGO data for Linux and windows-x86#51656

Merged
agocke merged 7 commits intodotnet:mainfrom
agocke:ingest-pgo
Apr 25, 2021
Merged

Use newest PGO data for Linux and windows-x86#51656
agocke merged 7 commits intodotnet:mainfrom
agocke:ingest-pgo

Conversation

@agocke
Copy link
Member

@agocke agocke commented Apr 21, 2021

This data was previously held back because we weren't producing
builds with Linux or windows-x86 but now we are. The Linux builds
now also only produce a single profdata file, coreclr.profdata,
which should contain all the profile information for all the
libraries in the runtime.

This data was previously held back because we weren't producing
builds with Linux or windows-x86 but now we are. The Linux builds
now also only produce a single profdata file, coreclr.profdata,
which should contain all the profile information for all the
libraries in the runtime.
@ghost
Copy link

ghost commented Apr 21, 2021

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.

Copy link
Member

@brianrob brianrob left a comment

Choose a reason for hiding this comment

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

LGTM. Am I correct that with this change, if the PGO data doesn't get downloaded properly that the build will fail? If so, that is a good change.

@agocke
Copy link
Member Author

agocke commented Apr 22, 2021

@brianrob In my local testing, yes, that's the behavior.

@agocke agocke closed this Apr 22, 2021
@agocke agocke reopened this Apr 22, 2021
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Unfortunately, we need to allow for builds that do not have pgo data.

In particular source builds do not use pgo on any platform, and this is encoded in the logic where we don't attempt to restore pgo data on those builds, and the logic in cmake where missing pgo data is tolerated.

@agocke
Copy link
Member Author

agocke commented Apr 23, 2021

Change reverted, but I wonder if we could just have source build pass -nopgooptimize or something.

@brianrob
Copy link
Member

Official builds pass an enforcepgo flag to make sure that PGO was successful (on Windows at least). I agree that it would be good to be intentional about whether or not we expect PGO data to be present and used. That way if something goes wrong when we do expect it to be present, we can fail, rather than just falling back to not using it.

@agocke
Copy link
Member Author

agocke commented Apr 23, 2021

Looks like there's something busted here with the MSBuild win-x86 instrumentation. I'll try to figure out what's different.

@agocke agocke merged commit 90f97ac into dotnet:main Apr 25, 2021
@agocke agocke deleted the ingest-pgo branch April 25, 2021 01:18
@DrewScoggins
Copy link
Member

DrewScoggins commented Apr 26, 2021

DrewScoggins/performance-2#5369

We saw some major improvements in Crossgen scenarios, about 8%.

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants