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

Stop passing StandardOptimizationData.mibc to crossgen2 multiple times for SPC #67458

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

jakobbotsch
Copy link
Member

When invoking crossgen2 we are currently passing the mibc file 4 times, once for each scenario we have PGO data for. It means that the counts the JIT sees for SPC are inflated by 4x.

Note that we are using raw counts in the JIT that might be affected by this change:

//------------------------------------------------------------------------
// fgHaveSufficientProfileData: check if profile data is available
// and is sufficient enough to be trustful.
//
// Returns:
// true if so
//
// Note:
// See notes for fgHaveProfileData.
//
bool Compiler::fgHaveSufficientProfileData()
{
if (!fgHaveProfileData())
{
return false;
}
if ((fgFirstBB != nullptr) && (fgPgoSource == ICorJitInfo::PgoSource::Static))
{
const weight_t sufficientSamples = 1000;
return fgFirstBB->bbWeight > sufficientSamples;
}
return true;
}

However it is not right to multiply this value by 0.25x either as I believe for other framework DLLs we are passing StandardOptimizationData.mibc just once. So I am hoping to see if this has ill effects on SPC from the perf legs. @EgorBo's original PR #55096 already has data showing that most hot methods have more than 5000 counts, so hopefully we will not see anything too bad.

cc @dotnet/jit-contrib @davidwrighton

@jakobbotsch jakobbotsch added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 1, 2022
@ghost ghost assigned jakobbotsch Apr 1, 2022
@ghost
Copy link

ghost commented Apr 1, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

When invoking crossgen2 we are currently passing the mibc file 4 times, once for each scenario we have PGO data for. It means that the counts the JIT sees for SPC are inflated by 4x.

Note that we are using raw counts in the JIT that might be affected by this change:

//------------------------------------------------------------------------
// fgHaveSufficientProfileData: check if profile data is available
// and is sufficient enough to be trustful.
//
// Returns:
// true if so
//
// Note:
// See notes for fgHaveProfileData.
//
bool Compiler::fgHaveSufficientProfileData()
{
if (!fgHaveProfileData())
{
return false;
}
if ((fgFirstBB != nullptr) && (fgPgoSource == ICorJitInfo::PgoSource::Static))
{
const weight_t sufficientSamples = 1000;
return fgFirstBB->bbWeight > sufficientSamples;
}
return true;
}

However it is not right to multiply this value by 0.25x either as I believe for other framework DLLs we are passing StandardOptimizationData.mibc just once. So I am hoping to see if this has ill effects on SPC from the perf legs. @EgorBo's original PR #55096 already has data showing that most hot methods have more than 5000 counts, so hopefully we will not see anything too bad.

cc @dotnet/jit-contrib @davidwrighton

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Apr 1, 2022

I've checked locally this reduces R2R'd corelib size from 10.887 Mb to 10.852 Mb (-0.3%) in --Ot mode which means it inlines slightly less now but it's expected and looks fine to me

@jakobbotsch
Copy link
Member Author

The build failure seems to be because the .mibc file does not exist there, and the build previously did not have any files in OptimizationMibcFiles. @davidwrighton do you know if it is expected that we are currently not passing any .mibc file when crossgenning SPC in the "Build Linux x64 release SourceBuild" pipeline? E.g. the invocation for a previous passing build of the pipeline is:

    /__w/1/s/artifacts/source-build/self/src/dotnet.sh /__w/1/s/artifacts/source-build/self/src/artifacts/bin/coreclr/Linux.x64.Release/crossgen2/crossgen2.dll -o:/__w/1/s/artifacts/source-build/self/src/artifacts/bin/coreclr/Linux.x64.Release/System.Private.CoreLib.dll -r:/__w/1/s/artifacts/source-build/self/src/artifacts/bin/coreclr/Linux.x64.Release/IL/*.dll --targetarch:x64  --embed-pgo-data -O /__w/1/s/artifacts/source-build/self/src/artifacts/bin/coreclr/Linux.x64.Release/IL/System.Private.CoreLib.dll --perfmap-format-version:1 --perfmap --perfmap-path:/__w/1/s/artifacts/source-build/self/src/artifacts/bin/coreclr/Linux.x64.Release/

@jakobbotsch
Copy link
Member Author

Also cc @dotnet/source-build-contrib, is it expected that we are not passing PGO data in the SourceBuild pipeline?

@crummel
Copy link
Contributor

crummel commented Apr 5, 2022

Yes, the PGO data is considered a prebuilt binary that isn't allowed to be used in the open-source builds. We've talked about ways around this but haven't had the resources to really fix it yet.

@jakobbotsch jakobbotsch requested a review from crummel April 7, 2022 12:48
@jakobbotsch
Copy link
Member Author

Failure is #58927

@jakobbotsch jakobbotsch merged commit d1a2852 into dotnet:main Apr 12, 2022
@jakobbotsch jakobbotsch deleted the pgo-profile-fixes branch April 12, 2022 11:38
@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants