Repository navigation
Test System.Runtime.Serialization.Xml.ReflectionOnly.Tests stack overflowing in Windows x64 release for libraries-pgo pipeline #100246
Description
Some tests in System.Runtime.Serialization.Xml.ReflectionOnly.Tests are failing due to a stack overflow on windows x64 for libraries-pgo pipeline.
set DOTNET_TieredCompilation=1
set DOTNET_TC_OnStackReplacement=1
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TC_OnStackReplacement_InitialCounter=1
set DOTNET_OSR_HitLimit=2
set DOTNET_JitRandomOnStackReplacement=15
C:\h\w\A5B1092D\w\B8060970\e>"C:\h\w\A5B1092D\p\dotnet.exe" exec --runtimeconfig System.Runtime.Serialization.Xml.ReflectionOnly.Tests.runtimeconfig.json --depsfile System.Runtime.Serialization.Xml.ReflectionOnly.Tests.deps.json xunit.console.dll System.Runtime.Serialization.Xml.ReflectionOnly.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing
Discovering: System.Runtime.Serialization.Xml.ReflectionOnly.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Serialization.Xml.ReflectionOnly.Tests (found 260 of 261 test cases)
Starting: System.Runtime.Serialization.Xml.ReflectionOnly.Tests (parallel test collections = on [4 threads], stop on fail = off)
Stack overflow.
at System.Xml.ValueHandle.GetCharsText()
at System.Xml.ValueHandle.GetString()
Activity
AndyAyersMS commentedon Mar 26, 2024
Likely OSR related, as this stress mode forces a whole lot of methods to go through OSR.
@dotnet/jit-contrib -- possibly a good issue for somebody who has never debugged an OSR problem. Feel free to assign yourself.
amanasifkhalid commentedon Apr 1, 2024
I haven't been able to reproduce this locally -- @AndyAyersMS is
DOTNET_JitRandomOnStackReplacementdeterministic?AndyAyersMS commentedon Apr 1, 2024
Yes and no -- the "random" placement of patchpoints is deterministic; whether or how often the patchpoints get hit depends on the determinism of the test. And xunit by default is intentionally randomizing, so it's often the case that you need to run a few hundred times to be sure you don't have a repro.
amanasifkhalid commentedon Jul 19, 2024
Using a checked JIT built from
release/9.0-preview5, I cannot reproduce this after hundreds of runs. libraries-pgo hasn't hit this recently, either.amanasifkhalid commentedon Jul 22, 2024
I spoke too soon... The last run of
libraries-pgohit this issue again.AndyAyersMS commentedon Jul 29, 2024
Also here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=758261&view=ms.vss-test-web.build-test-results-tab
amanasifkhalid commentedon Aug 9, 2024
After discussing with @AndyAyersMS, I don't think this is due to an OSR bug. This failure only affects Windows x64, which is noteworthy for a few reasons:
My new hypothesis is this test has especially deep call stacks, and the additional memory pressure from OSR patchpoints causes intermittent stack overflows, particularly when methods with patchpoints are especially hot in a recursive call chain, and we don't have time to tier these methods up before overflowing.
I tried reproducing these stack overflows on Linux x64, since it's easier to modify the default stack size. First, I tried constraining the stack size significantly, just to see if these large call chains would become a problem without any OSR stress. With a stack size of ~700 KB, I can reliably reproduce the stack overflow without any OSR stress modes set. Next, I set the stack size to 1 MB to match Windows, and set
DOTNET_JitRandomOnStackReplacement=100to maximize frame bloat. I couldn't consistently reproduce the stack overflow, but if I drop the stack size to 980 KB, I hit it every time (I suspect this discrepancy is due to tiering sometimes taking too long?).In a particular run,
System.Runtime.Serialization.ReflectionReader:ReflectionReadValueshows up in the problematic stack trace multiple times. Without any OSR stress modes, we don't insert patchpoints into this method, and the frame info looks like this:And with
DOTNET_JitRandomOnStackReplacement=100set, we see the patchpoint bloating the frame:Thus, I don't think this is an actual OSR bug, but rather the stress mode exhausting the machine's stack space. @AndyAyersMS thanks for your help in investigating this! Do you suggest we just exclude this test from OSR stress? I imagine we can do that similarly to how we exclude tests from GCStress?
AndyAyersMS commentedon Aug 11, 2024
There is no OSR stress exclude mechanism right now, and I don't know if we have a way of exempting libraries tests from gc stress either...?
I wouldn't expect tier0+patchpoint frames to get all that much bigger than normal tier0 methods. Though I suppose there is extra register save space (x64 only) and the slot for the counter. Maybe that's enough to tip this case over. Would be good to look at the mixture of Tier0 vs OSR methods on the stack at the point of overflow.
Also it looks like in the CI run above that the thread that hits overflow is in the patchpoint helper at the point it overflows.
It would be good to get a more accurate picture of this stack (you should hopefully be able to do this from the dump). It might make sense for the patchpoint helper to try and minimize the work it does (eg return without trying to trigger jitting and/or transitioning) if there is very limited stack available.
Or if this is really mostly managed code, we might want to ask the libraries folks if it is normal that we see such deep recursion -- generally this is just asking for trouble.
amanasifkhalid commentedon Aug 12, 2024
Sure. Looking at the following group of recursive calls in the offending call stack, every method was in Tier0, and had patchpoints inserted (I tweaked
JitDisasmSummaryto indicate ifCompiler::doesMethodHavePatchpointsis true for a method):Looking at this under WinDbg, execution seems to have been halted here. There is an inlining hint in the signature of
BaseContract::DoChecks, but I'm guessing it wasn't inlined, and this function call exhausted any remaining stack space, triggering the stack overflow.From offline discussion, the above code shape seems unlikely to try to use OSR outside of stress modes, and modifying the patchpoint helper to handle low-memory scenarios doesn't seem like the right solution. We don't think there's anything to "fix" here, though we don't seem to have any obvious options for opting this test out of OSR stress at the moment:
<JitOptimizationSensitive>in their proj files to opt out of MinOpts/JitStress modes. We could easily modify the test execution scripts to skip a test ifJitOptimizationSensitiveandDOTNET_JitRandomOnStackReplacementare both set, but this mechanism doesn't apply to libraries tests, anyway. @jkoritzinsky do you know if there are any similar mechanisms for opting libraries tests out of certain stress modes?DOTNET_JitRandomOnStackReplacementto reduce the odds of the JIT inserting patchpoints into methods like those in the above call stack, but that doesn't guarantee a fix, and we'll exercise OSR less across all tests.System.Runtime.Serialization.Xml.ReflectionOnly.Testsaltogether.jkoritzinsky commentedon Aug 12, 2024
@amanasifkhalid you can use
[SkipOnCoreCLR("https://github.com/dotnet/runtime/issues/100246", RuntimeTestModes.AnyJitOptimizationStress)]on the test method to get the same behavior asJitOptimizationSensitive. Otherwise you could add anActiveIssueattribute and have a property on the type that checks the relevant combination of environment variables for when to skip the test.amanasifkhalid commentedon Aug 13, 2024
@jkoritzinsky Thank you!
SkipOnCoreCLRlooks like it will work for our purposes. I don't think we need to be as heavy-handed asJitOptimizationSensitive-- maybe I can add a newRuntimeTestModefor random OSR, and exclude the offending test on that?jkoritzinsky commentedon Aug 13, 2024
We can add another mode, but that needs to be added in Arcade and in our XUnitWrapperGenerator. I think it would be easier/faster to add your own environment variable check with ConditionalFact.
JulieLeeMSFT commentedon Aug 13, 2024
Failed in runtime-coreclr libraries-pgo/20240811.1