enable osr stress tests for libraries and add fixes #64116

AndyAyersMS · 2022-01-21T21:28:19Z

Enables OSR stress for libraries.

This uncovered a few issues that are fixed with subsequent commits:

generics context reporting for OSR methods was broken, when the Tier0 method did not need to report the context
tail recursion detection was broken when the call in tail position was devirtualized.
with OSR enabled, logic for when we decide to switch a method from Tier0 to optimized was switching too many methods

ghost · 2022-01-21T21:28:25Z

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-Infrastructure-libraries`
Milestone:	-

AndyAyersMS · 2022-01-21T21:29:15Z

Seeing issues with libraries + osr for arm64 over in #63720, want to get a broader view.

AndyAyersMS · 2022-01-21T21:29:56Z

cc @dotnet/jit-contrib

BruceForstall

This seems fine to me, but to be more consistent with the coreclr tests, maybe there should be a "jit-experimental" testGroup (and pipeline) for libraries.

Or maybe at this point the OSR stuff should "graduate" to its own "osr" testGroup in both places?

AndyAyersMS · 2022-01-21T22:09:06Z

Or maybe at this point the OSR stuff should "graduate" to its own "osr" testGroup in both places?

Probably a good idea, but the combinatorics of all this start getting messy....

AndyAyersMS · 2022-01-21T22:10:39Z

/azp run runtime-coreclr libraries-pgo

azure-pipelines · 2022-01-21T22:10:54Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-01-22T02:52:45Z

/azp run runtime-coreclr libraries-pgo

azure-pipelines · 2022-01-22T02:52:59Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-01-25T03:03:22Z

Generics context is still not right in case where the OSR method needs to report this but the Tier0 method didn't need to. While we think we're saving this in the OSR prolog, genReportGenericContextArg assumes the arg registers have their proper values, and that's not the case for an OSR method.

Some options:

fetch the Tier0 frame this and save it in the OSR context slot. This would work for x64 but perhaps not for arm64. We'd need to do a mem-mem move and on arm64 we only have one scratch reg and may need it to form the offset to the Tier0 slot. Now if we happen to have this enregistered on entry we could reorder the OSR arg fetches and context slot init and so get around the register limitation, but that's not guaranteed....
report the Tier0 this frame slot as the context slot.
force the Tier0 method to always keep alive and report the context, report the context slot as live.

The last one seems the simplest so will do that....

… just use that

AndyAyersMS · 2022-01-25T17:31:58Z

/azp run runtime-coreclr libraries-pgo

azure-pipelines · 2022-01-25T17:32:15Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-01-25T18:35:38Z

Linux x64 debug failure:

  Starting:    System.Runtime.Experimental.Tests (parallel test collections = on, max threads = 2)
Process terminated. Assertion failed.
   at System.Number.FormatHalf(ValueStringBuilder& sb, Half value, ReadOnlySpan`1 format, NumberFormatInfo info) in /_/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs:line 723

Similar to #61359 -- probably not related to this PR.

AndyAyersMS · 2022-01-25T21:44:04Z

Still seeing some libraries test failures, looks like bad codegen. Let's see if we can catch it in coreclr tests.

AndyAyersMS · 2022-01-25T21:44:12Z

/azp run runtime-jit-experimental

azure-pipelines · 2022-01-25T21:44:28Z

Azure Pipelines successfully started running 1 pipeline(s).

* Dump IR after patchpoints phase, if any were added * Fix bug in recursive tail call detection -- if we devirtualize we need to update the method handle we use. This last change is the key bug fix -- OSR methods rely on getting this detection right so they know if they need to import the method entry block.

AndyAyersMS · 2022-01-26T17:50:01Z

Last commit fixed a subtle bug.

When we devirtualize a tail call we might fail to recognize that the tail call is now a recursive tail call, because we are checking the wrong method handle. For normal methods this just means we may not mark the block range as a loop (which is bad but rarely leads to bugs). But for OSR methods this is fatal because they need to use this to trigger importation of the method entry.

One method where this happens is System.Xml.Serialization.StructMapping:System.Xml.Serialization.INameScope.get_Item(System.String,System.String):System.Object:this from the xml serialization libraries tests (under random OSR stress, we put a patchpoint at offset 0x1b).

AndyAyersMS · 2022-01-26T18:51:33Z

Still think the enabling logic for OSR is off. Currently with QJFL=1, OSR=1 we will bail to opt for too many cases.

The logic with QJFL=1, OSR=1 should be

canGetTrappedAtTier0 = (has loop) || (has tail. prefix)
canBeBailedOutByOSR = !(hasLocalloc || isRPInvoke || has loop in handler)

optimize = canGetTrappedAtTier0 && !canBeBailedByOSR

but currently it's

!canBeBailedOutByOSR

So will fix this along with the other changes...

AndyAyersMS · 2022-01-26T19:58:17Z

/azp run runtime-coreclr libraries-pgo

azure-pipelines · 2022-01-26T19:58:31Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-01-26T21:38:28Z

x86 libraries test failing

  Starting:    System.Diagnostics.PerformanceCounter.Tests (parallel test collections = on, max threads = 2)
    System.Diagnostics.Tests.PerformanceCounterTests.PerformanceCounter_CreateCounter_MultiInstanceReadOnly [FAIL]
      System.AggregateException : One or more errors occurred. (Category 'PerformanceCounter_CreateCounter_MultiInstanceReadOnly_Category' does not exist.) (Category 'PerformanceCounter_CreateCounter_MultiInstanceReadOnly_Category' does not exist.) (Category 'PerformanceCounter_CreateCounter_MultiInstanceReadOnly_Category' does not exist.)

Almost certainly unrelated as this change should be no-diff for x86.

AndyAyersMS · 2022-01-26T21:58:08Z

/azp run runtime-jit-experimental

azure-pipelines · 2022-01-26T21:58:23Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-01-26T22:09:56Z

@EgorBo can you make sure to look over the changes in this PR for the OSR enabling/fallback logic? Also perhaps remeasure some of the TE benchmarks (with QFJL=1 OSR=1, since this PR doesn't enable those by default)...?

I think we were needlessly optimizing too many methods when OSR was turned on....

AndyAyersMS · 2022-01-26T22:28:35Z

Libraries stress test still finding issues.

EgorBo · 2022-01-26T22:35:12Z

@EgorBo can you make sure to look over the changes in this PR for the OSR enabling/fallback logic? Also perhaps remeasure some of the TE benchmarks (with QFJL=1 OSR=1, since this PR doesn't enable those by default)...?

I think we were needlessly optimizing too many methods when OSR was turned on....

Sure!
Will schedule a run for this PR overnight

AndyAyersMS · 2022-01-27T02:50:13Z

Summary of the latest batch of issues:

x64 has a problem if we suspend a thread in the middle of an OSR epilog. Unwind gets confused because it is examining the actual epilog code and sees the second SP adjust and decides we must not be in an epilog. So it uses the full set of prolog unwind codes (in reverse) to restore context. But we are in the middle of the epilog so this gives corrupted context.

Assert failure(PID 8132 [0x00001fc4], Thread: 8112 [0x1fb0]): ((FARPROC) (TADDR)m_pvHJRetAddr) != NULL

arm64 has a problem with a misaligned register load in an OSR prolog. Fixed by fix arm64 genInstrWithConstant #64368.

Assert failure(PID 28 [0x0000001c], Thread: 48 [0x0030]): Assertion failed 'isValidGeneralDatasize(size)'

arm64 has the known issue with a too-large prolog (GitHub_17777)
arm64 hits assert that I can't repro yet

Assert failure(PID 27 [0x0000001b], Thread: 33 [0x0021]): Assertion failed 'fgReachable(begBlk, endBlk)' in 'MS.Internal.Xml.XPath.FilterQuery:MatchNode(System.Xml.XPath.XPathNavigator):System.Xml.XPath.XPathNavigator:this' during 'Update flow graph opt pass' (IL size 444)

    File: /__w/1/s/src/coreclr/jit/optimizer.cpp Line: 167
    Image: /root/helix/work/correlation/dotnet

arm64 runtime failure (windows). Haven' tried this on windows but on Linux this does not hit any patchpoints or create any OSR methods, so seemingly unrelated?

JIT\Regression\JitBlue\GitHub_21990

AndyAyersMS · 2022-01-27T22:50:19Z

Needed to merge up to resolve a conflict and pick up a fix.

I think we've probably gotten what we can out of this, the x64 unwind issue is the big remaining open. So after merging up I'll just let the default CI run...

@dotnet/jit-contrib PTAL

BruceForstall · 2022-01-28T00:25:30Z

src/coreclr/jit/compiler.hpp

@@ -1986,13 +1986,18 @@ inline bool Compiler::lvaKeepAliveAndReportThis()
    // the VM requires us to keep the generics context alive or it is used in a look-up.
    // We keep it alive in the lookup scenario, even when the VM didn't ask us to,
    // because collectible types need the generics context when gc-ing.
+    //
+    // Methoods that can inspire OSR methods must always report context as live


typo: Methoods

Thanks. Will fix in subsequent PR.

add osr plus stress tests for libraries

a8babb3

dotnet-issue-labeler bot added the area-Infrastructure-libraries label Jan 21, 2022

ghost assigned AndyAyersMS Jan 21, 2022

BruceForstall approved these changes Jan 21, 2022

View reviewed changes

fix context reporting issue

a8b8ab1

have tier0 method always report generics context, then OSR method can…

3766dbc

… just use that

remove some debug scaffolding

bfd1dd8

refine OSR enabling/backoff to opt logic

2a04aae

AndyAyersMS changed the title ~~add osr plus stress tests for libraries~~ enable osr stress tests for libraries and add fixes Jan 26, 2022

AndyAyersMS mentioned this pull request Jan 27, 2022

fix arm64 genInstrWithConstant #64368

Merged

AndyAyersMS added 2 commits January 27, 2022 14:44

merge main into OsrLibrariesTesting

70387a3

reorder phase whitelist to more closely match phase order

9185609

BruceForstall approved these changes Jan 28, 2022

View reviewed changes

AndyAyersMS merged commit 479ee51 into dotnet:main Jan 28, 2022

AndyAyersMS mentioned this pull request Jan 28, 2022

On Stack Replacement Next Steps #33658

Open

72 tasks

BruceForstall mentioned this pull request Jan 28, 2022

Assertion failed 'ppInfo->HasKeptAliveThis()' #64477

Closed

AndyAyersMS mentioned this pull request Feb 2, 2022

Dynamic PGO work planned for .NET 7 #64659

Closed

10 tasks

ghost locked as resolved and limited conversation to collaborators Feb 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable osr stress tests for libraries and add fixes #64116

enable osr stress tests for libraries and add fixes #64116

AndyAyersMS commented Jan 21, 2022 •

edited

Loading

ghost commented Jan 21, 2022

AndyAyersMS commented Jan 21, 2022

AndyAyersMS commented Jan 21, 2022

BruceForstall left a comment

AndyAyersMS commented Jan 21, 2022

AndyAyersMS commented Jan 21, 2022

azure-pipelines bot commented Jan 21, 2022

AndyAyersMS commented Jan 22, 2022

azure-pipelines bot commented Jan 22, 2022

AndyAyersMS commented Jan 25, 2022 •

edited

Loading

AndyAyersMS commented Jan 25, 2022

azure-pipelines bot commented Jan 25, 2022

AndyAyersMS commented Jan 25, 2022

AndyAyersMS commented Jan 25, 2022

AndyAyersMS commented Jan 25, 2022

azure-pipelines bot commented Jan 25, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

EgorBo commented Jan 26, 2022

AndyAyersMS commented Jan 27, 2022 •

edited

Loading

AndyAyersMS commented Jan 27, 2022

BruceForstall Jan 28, 2022

AndyAyersMS Jan 28, 2022

enable osr stress tests for libraries and add fixes #64116

enable osr stress tests for libraries and add fixes #64116

Conversation

AndyAyersMS commented Jan 21, 2022 • edited Loading

ghost commented Jan 21, 2022

AndyAyersMS commented Jan 21, 2022

AndyAyersMS commented Jan 21, 2022

BruceForstall left a comment

Choose a reason for hiding this comment

AndyAyersMS commented Jan 21, 2022

AndyAyersMS commented Jan 21, 2022

azure-pipelines bot commented Jan 21, 2022

AndyAyersMS commented Jan 22, 2022

azure-pipelines bot commented Jan 22, 2022

AndyAyersMS commented Jan 25, 2022 • edited Loading

AndyAyersMS commented Jan 25, 2022

azure-pipelines bot commented Jan 25, 2022

AndyAyersMS commented Jan 25, 2022

AndyAyersMS commented Jan 25, 2022

AndyAyersMS commented Jan 25, 2022

azure-pipelines bot commented Jan 25, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

AndyAyersMS commented Jan 26, 2022

EgorBo commented Jan 26, 2022

AndyAyersMS commented Jan 27, 2022 • edited Loading

AndyAyersMS commented Jan 27, 2022

BruceForstall Jan 28, 2022

Choose a reason for hiding this comment

AndyAyersMS Jan 28, 2022

Choose a reason for hiding this comment

AndyAyersMS commented Jan 21, 2022 •

edited

Loading

AndyAyersMS commented Jan 25, 2022 •

edited

Loading

AndyAyersMS commented Jan 27, 2022 •

edited

Loading