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

JIT: profile synthesis consistency checking and more #83068

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

AndyAyersMS
Copy link
Member

Add the ability to verify that the profile counts produced by synthesis are self-consistent, and optionally to assert if they're not. Disable checking if we know profile flow will be inconsistent (because of irreducible loops and/or capped cyclic probabilities).

Consistently ignore the likely flow out of handlers. Generally we model handlers as isolated subgraphs that do not contribute flow to the main flow graph. This is generally acceptable.

The one caveat is for flow into finallies. The plan here is to fix the weights for finallies up in a subsequent pass via simple scaling once callfinallies are introduced. Flow weights out of finallies will be ignored as the callfinally block will be modeled as always flowing to its pair tail.

Also add the ability to propagate the synthesized counts into the live profile data. This is mainly to facilitate using the MIBC comparison tools we have to assess how closely the synthesiszed data comes to actual PGO data.

Finally, enable the new synthesized plus checked modes in a few of our PGO pipelines.

Contributes to #82964.

Add the ability to verify that the profile counts produced by synthesis are
self-consistent, and optionally to assert if they're not. Disable checking
if we know profile flow will be inconsistent (because of irreducible loops
and/or capped cyclic probabilities).

Consistently ignore the likely flow out of handlers. Generally we model
handlers as isolated subgraphs that do not contribute flow to the main flow
graph. This is generally acceptable.

The one caveat is for flow into finallies. The plan here is to fix the weights
for finallies up in a subsequent pass via simple scaling once callfinallies
are introduced. Flow weights out of finallies will be ignored as the
callfinally block will be modeled as always flowing to its pair tail.

Also add the ability to propagate the synthesized counts into the live profile
data. This is mainly to facilitate using the MIBC comparison tools we have
to assess how closely the synthesiszed data comes to actual PGO data.

Finally, enable the new synthesized plus checked modes in a few of our PGO
pipelines.

Contributes to dotnet#82964.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 7, 2023
@ghost ghost assigned AndyAyersMS Mar 7, 2023
@ghost
Copy link

ghost commented Mar 7, 2023

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

Issue Details

Add the ability to verify that the profile counts produced by synthesis are self-consistent, and optionally to assert if they're not. Disable checking if we know profile flow will be inconsistent (because of irreducible loops and/or capped cyclic probabilities).

Consistently ignore the likely flow out of handlers. Generally we model handlers as isolated subgraphs that do not contribute flow to the main flow graph. This is generally acceptable.

The one caveat is for flow into finallies. The plan here is to fix the weights for finallies up in a subsequent pass via simple scaling once callfinallies are introduced. Flow weights out of finallies will be ignored as the callfinally block will be modeled as always flowing to its pair tail.

Also add the ability to propagate the synthesized counts into the live profile data. This is mainly to facilitate using the MIBC comparison tools we have to assess how closely the synthesiszed data comes to actual PGO data.

Finally, enable the new synthesized plus checked modes in a few of our PGO pipelines.

Contributes to #82964.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

SPMI passes with synthesized counts and checking enabled. So basic algorithm is looking pretty solid (at least running where it does, super early).

As for how accurate it is given the very simple heuristics, here is one take: comparing a synthesized MIBC with a measured one using interlocked PGO on TE MvcPlaintext:

Comparing methods with matching flow-graphs
The overlap of the block counts break down as follows:
100% █████▉ (3.4%)
>95% ███████▊ (4.5%)
>90% ███████▌ (4.3%)
>85% ██████▍ (3.7%)
>80% ████████████ (6.9%)
>75% ████████████████████████████▉ (16.6%)
>70% ███████████████████████████████████▋ (20.4%)
>65% ███████████▋ (6.7%)
>60% █████████▌ (5.4%)
>55% █████████████▉ (8.0%)
>50% ██████▌ (3.8%)
>45% ██████▌ (3.8%)
>40% ███████▊ (4.5%)
>35% ██▍ (1.4%)
>30% ███ (1.8%)
>25% ██▌ (1.5%)
>20% █▋ (1.0%)
>15% ▋ (0.4%)
>10% █▎ (0.8%)
> 5% █▍ (0.8%)
> 0% ▉ (0.6%)
The average overlap is 68.21% for the 1413 methods with matching flow graphs and profile data
The mean squared error is 1381.32
There are 220/1413 methods with overlaps < 50%:

This looks fairly good, but may be misleading as there are many simple methods.

@AndyAyersMS
Copy link
Member Author

Disable checking if we know profile flow will be inconsistent (because of irreducible loops and/or capped cyclic probabilities).

Irreducible loops seem to be somewhat common in async methods. Around 700 cases in ASP.NET collection.

Probability caps are rare in actual (non-test code) -- there are only a handful (less than 10) in the ASP.NET collection.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM


inline constexpr ProfileChecks operator ~(ProfileChecks a)
{
return (ProfileChecks)(~(unsigned int)a);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have a CHECK_ALL that contains all the legal bits (currently 0xF) and have this be:

Suggested change
return (ProfileChecks)(~(unsigned int)a);
return (ProfileChecks)(CHECK_ALL & ~(unsigned int)a);

to prevent returning bits that are not "legal" ProfileChecks bits.

Could validate/cap the value of JitConfig.JitProfileChecks() this way, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all boilerplate for our common enum class approach, so we'd probably want to try and handle them all this way.

Since enum class is (somewhat) type safe seems like we could just mask when converting from int or other similar types?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I hadn't noticed that other similar cases had also defined operator~() without this. A minor thing, to be sure.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Mar 7, 2023

x86 failure looks like an AV in the xunit test host runtime? Exit code is 0xC0000005. Not much to go on. Looks like 4 other similar failures in the past week. Opened #83098.

C:\h\w\AFF209B1\w\AA51092E\e>"C:\h\w\AFF209B1\p\dotnet.exe" exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Net.Http.Functional.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Http.Functional.Tests (found 1351 of 1634 test cases)
  Starting:    System.Net.Http.Functional.Tests (parallel test collections = on, max threads = 2)
    System.Net.Http.Functional.Tests.SyncHttpHandler_HttpClientHandler_Authentication_Test.Credentials_ServerUsesWindowsAuthentication_Success [SKIP]
      Condition(s) not met: "IsWindowsServerAvailable"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClient_SelectedSites_Test.RetrieveSite_Succeeds [SKIP]
      Condition(s) not met: "IsSelectedSitesTestEnabled"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClient_SelectedSites_Test.RetrieveSite_Debug_Helper [SKIP]
      Condition(s) not met: "IsSelectedSitesTestEnabled"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientEKUTest.HttpClient_ClientEKUServerAuth_Fails [SKIP]
      Condition(s) not met: "CanTestCertificates"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientEKUTest.HttpClient_NoEKUServerAuth_Ok [SKIP]
      Condition(s) not met: "CanTestCertificates"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientEKUTest.HttpClient_NoEKUClientAuth_Ok [SKIP]
      Condition(s) not met: "CanTestCertificates"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientEKUTest.HttpClient_ServerEKUClientAuth_Fails [SKIP]
      Condition(s) not met: "CanTestCertificates"
    System.Net.Http.Functional.Tests.SyncHttpHandler_HttpClientHandler_Authentication_Test.Proxy_DomainJoinedProxyServerUsesKerberos_Success [SKIP]
      Condition(s) not met: "IsDomainJoinedServerAvailable"
----- end Tue 03/07/2023  4:57:30.26 ----- exit code -1073741819 ---------------------------------------------------------

Other failures are known/timeouts.

@AndyAyersMS AndyAyersMS merged commit 168a72e into dotnet:main Mar 7, 2023
@AndyAyersMS AndyAyersMS mentioned this pull request Mar 7, 2023
21 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Apr 6, 2023
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.

2 participants