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

Optimize JIT_ClassProfile32 #82014

Merged
merged 3 commits into from
Feb 18, 2023
Merged

Optimize JIT_ClassProfile32 #82014

merged 3 commits into from
Feb 18, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 12, 2023

I'm still seeing JIT_ClassProfile32 being top5 hottest functions in the TE traces + was surprised to see it for Paint.NET's traces (forced to stay in tier0-instrumented).

image

New codegen:

image

This PR does:

  1. optimizes classProfile->Count++ operation. volatile seems unnecessary here
  2. force inline CheckSample into JIT_ClassProfile32
  3. Little massage to produce better codegen on MSVC (other compilers do a better job as is)
  4. Replaces pMT->GetLoaderAllocator()->IsCollectible() with pMT->Collectible()

(mostly written by @jakobbotsch)

Open questions

  1. It seems like we need to collect Native PGO with DOTNET_TieredPGO=1 to produce a better NPGO coverage.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM! BTW, when I looked at it the FORCEINLINE was not required for MSVC to inline CheckSample, but still seems fine to me to have it given that we know it's very hot.

Would also like to know what the intention of the volatile increment was since that is being removed (cc @AndyAyersMS)

@AndyAyersMS
Copy link
Member

We might also look at implementing something closer to the true reservoir sampling via if ((x % count) >= S) or if you don't want that unknown value modulus, at least decreasing the probability of an update once count is large.

Would also like to know what the intention of the volatile increment was since that is being removed (cc @AndyAyersMS)

Doesn't look like it is needed. I don't remember why I had it that way.

@AndyAyersMS
Copy link
Member

@EgorBo did you collect any data for this one?

@EgorBo
Copy link
Member Author

EgorBo commented Feb 18, 2023

@EgorBo did you collect any data for this one?

I did, the improvement was quite small, I assume, mostly because the main issue is in classProfile->Count++ cache contention

@EgorBo
Copy link
Member Author

EgorBo commented Feb 18, 2023

/azp list

@azure-pipelines

This comment was marked as resolved.

@EgorBo
Copy link
Member Author

EgorBo commented Feb 18, 2023

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Feb 18, 2023

Failure is #82275

@EgorBo EgorBo merged commit edce5ea into dotnet:main Feb 18, 2023
@EgorBo EgorBo deleted the improve-classprofile32 branch February 18, 2023 21:47
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2023
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.

3 participants