Unwanted GC root in unused struct on stack

[antonfirsov]

Seems like this is the root cause for #35846. Considering an infinite event loop like the following:

private void EventLoop()
{
    while (true)
    {
        WaitForSocketEvents();
        foreach (var socketEvent in _buffer)
        {
            if (_socketWrappers != null)
            {
                var contextWrapper = _socketWrappers[0];
                var ev = new SocketIOEvent(contextWrapper.Context, SocketEvents.Read);
                _eventQueue.Enqueue(ev);

                ev = default;
                contextWrapper = default;
            }
        }
    }
}

When a local struct holds a reference to a heap object, it remains a GC Root, even if it goes out the scope. (IP is waiting infinitely at WaitForSocketEvents().)

Self-contained repro here:
https://github.com/antonfirsov/SocketEngineCodegenRepro

I was able to reproduce this in both Debug and Release, and observe the GC root with SOS, just like @kouvel did in the original issue: #35846 (comment)

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @Maoni0
Notify danmosemsft if you want to be subscribed.

[benaadams]

Does the loop compile in Tier0

i.e. if you add AggressiveOptimization does it help?

[MethodImpl(MethodImplOptions.AggressiveOptimization)]
private void EventLoop()

[jkotas]

When a local struct holds a reference to a heap object, it remains a GC Root, even if it goes out the scope

This behavior is by design. The codegen is free to arbitrarily extend the lifetime of locals.

Check these discussions for details:

#9328
#6157 (comment)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[kouvel]

The part I found odd in that function is that in optimized code contextWrapper = default; is optimized out by the JIT, indicating that it must not be live anymore, but it is still kept alive around the loop and since the struct is not cleared it holds on to a SocketAsyncContext. If the lifetime of the struct were extended then at least the clearing of it should be kept around.

[kouvel]

For longer term it may be feasible to separate the handling of events into a NoInline method to make it work the expected way deterministically in Debug and Release builds.

[kouvel]

handling of events into a NoInline method

Ah I just saw on the other issue that you already tried that and saw a regression. It seems a bit surprising that one call per loop iteration would cause a perf regression, how bad is it and are you sure it's outside noise range?

[jkotas]

The part I found odd in that function is that in optimized code contextWrapper = default; is optimized out by the JIT

The JIT is free to create temporary copies of values that are outside your control. I suspect that it is likely happening in this case.

make it work the expected way deterministically

Note that Mono has conservative GC. The NoInline trick does not solve this with non-deterministic GC.

Is the extension of the lifetime causing performance issues or functionality issues? If it is causing functionality issues, it would be a good idea to rethink the design to be robust against arbitrary lifetime extensions.

[kouvel]

Oh I see. The issue is that the SocketAsyncContext that remains alive in turn keeps the associated Socket object alive. So when the epoll thread is waiting it may indefinitely prevent a socket object from being finalized. There is a unit test that verifies that after using a socket object and not disposing it, that it gets finalized in reasonable time. It seems like a reasonable test but I'm not sure how important it is to retain that functionality.

[antonfirsov]

Looks like we should solve this within the constraints of the current runtime behavior, so I'm closing this issue. @jkotas thanks for the feedback!

@kouvel I suggest to move back further discussions to #35846.