RyuJIT GC reporting bug when method is inlined

[mjsabby]

    internal sealed class NativeResource : IDisposable
    {
        ~NativeResource() { this.Dispose(); }
        public void Dispose() { Environment.Exit(0); } // gets inlined
    }

    internal sealed class Bing_JITGCReporting_MediumHaul
    {
        public static void Main(string[] args)
        {
            var server = new NativeResource();
            CreateSomeGarbage();
            Thread.Sleep(Timeout.Infinite);
            server.Dispose();
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        private static void CreateSomeGarbage()
        {
            Task.Factory.StartNew(() =>
            {
                for (;;)
                {
                    Console.WriteLine(new object());
                }
            });
        }
    }

This program eventually terminates. If you mark Dispose with NoInlining, the program runs indefinitely.

This is a medium haul test that became flakey when I upgraded to recent bits of CoreCLR; this is not quite the code we had, it was intermittent since the test often finished before a GC kicked in, but this minimal repro is consistent.

I think RyuJIT should extend the reported lifetimes when it inlines an instance method at the call site, that may not be using the this pointer, but the type has a destructor. I can't prove it, but I think the non-destructor case has no observable difference so reporting accurate lifetimes should be fine.

This is a regression from either super early bits of CoreCLR, or code that was ported from .NET 4.5.2, I can't say for sure.

[mjsabby]

cc @cmckinsey

[mikedn]

If I understand correctly you expect this program to never terminate. But it does so in .NET 4.6.1 on both x86 and x64 so I'm not sure what this has to do with RyuJIT. Besides, why would this be a regression in JIT given that the test is basically wrong?

[mjsabby]

I'm not sure what this has to do with RyuJIT

The behavior is different with SET COMPlus_UseLegacyJit=1.

why would this be a regression in JIT given that the test is basically wrong?

The test reproduces an observable difference in behavior between RyuJIT and JIT64. While the minimal repro doesn't look that great, it was a nasty one to find.

It's up to @cmckinsey and team to decide if this lifetime reporting difference is something we can live with.

[RussKeldorph]

Calling it a bug for now, but it does feel like something that needs a judgment call.

[RussKeldorph]

@dotnet/jit-contrib @jkotas

[jkotas]

The behavior you are seeing is by design. If your program requires that the object is not collected before certain point, you need to use GC.KeepAlive to keep the reference alive up to that point.

[mjsabby]

I'm aware of GC.KeepAlive. This issue is for the reporting difference between RyuJIT and JIT64. I'm asking us to make a decision on this given existing code that implicitly depends on this behavior, rightly or wrongly.

[jkotas]

I do not think there is anything to do for CoreCLR. CoreCLR is not meant to be bug-for-bug compatible with JIT64.

I agree that we may consider quirking this for full framework if it repros there. There are several compatibility quirks in the JIT for full framework already.

[cmckinsey]

Just for reference: https://msdn.microsoft.com/en-us/library/system.object.finalize%28v=vs.110%29.aspx

•The exact time when the finalizer executes is undefined. To ensure deterministic release of resources for instances of your class, implement a Close method or provide a IDisposable.Dispose implementation.

Essentially the order of finalizer execution is undefined, as in you cannot rely on when their side-effects take place. From this perspective your test is invalid in that its behavior is different depending upon when the finalizers side-effect takes place relative to the Thread.Sleep call.

On a lower level, it is a pretty essential aspect of code-generation performance that the JIT have the ability to lengthen and shorten lifetimes as long as it continues to report the lifetimes when their live. In this case the lifetime was unnecessary and so the JIT is free to reclaim it (hence triggering the finalizer earlier during GC). The fact that JIT64 on Desktop happened to keep the lifetime around is pure coincidence and not something ever defined to be reliable from a compatibility perspective. In fact I can get JIT64 to match RyuJIT behavior by adding MethodImplOptions.AggressiveInlining to the Dispose method. There is nothing in JIT64 that guarantees this behavior. JIT32 also matches the RyuJIT behavior on x86. So basically there is no guaranteed that in other similar kinds of situations that this wouldn't have happened with JIT64. Also, there's nothing to say RyuJIT in a different situation would not have inlined the method (say at MinOpts) and such a case work in the future.

So my conclusion is that this is a bad test case. For .NET Core we would not consider quarking as an option. @mjsabby we can discuss offline regarding Desktop compatibility. Given this I'm closing this issue.

Thanks everyone.