Repository navigation
Unwanted GC root in unused struct on stack #37064
Description
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)
Activity
Dotnet-GitSync-Bot commentedon May 27, 2020
I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.
benaadams commentedon May 27, 2020
Does the loop compile in Tier0
i.e. if you add
AggressiveOptimizationdoes it help?jkotas commentedon May 27, 2020
This behavior is by design. The codegen is free to arbitrarily extend the lifetime of locals.
Check these discussions for details:
#9328
#6157 (comment)
kouvel commentedon May 27, 2020
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 aSocketAsyncContext. If the lifetime of the struct were extended then at least the clearing of it should be kept around.kouvel commentedon May 27, 2020
For longer term it may be feasible to separate the handling of events into a
NoInlinemethod to make it work the expected way deterministically in Debug and Release builds.kouvel commentedon May 27, 2020
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 commentedon May 27, 2020
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.
Note that Mono has conservative GC. The
NoInlinetrick 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 commentedon May 27, 2020
Oh I see. The issue is that the
SocketAsyncContextthat remains alive in turn keeps the associatedSocketobject 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 commentedon May 28, 2020
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.