[RuntimeAsync] Keep LoaderAllocator objects alive from continuations
#2812
Conversation
Add logic to make sure we keep the exposed loader allocator object alive when a runtime async function suspends. This adds two new JIT helpers `CORINFO_HELP_ALLOC_CONTINUATION_CLASS` and `CORINFO_HELP_ALLOC_CONTINUATION_METHOD` that store the `LoaderAllocator` associated with a class or method into the allocated continuation before returning it back to the JIT. There are a few cases where the JIT has to call the new helpers: 1. Any function loaded into a collectible ALC must keep the `LoaderAllocator` associated with the method being compiled alive 2. A shared generic function taking a `MethodDesc*` as the generic context must keep the `LoaderAllocator` assocated with that `MethodDesc*` alive 3. A shared generic function taking a `MethodTable*` as the generic context must similarly keep that one alive.
src/coreclr/jit/async.cpp
Outdated
| // TODO: Do we also need to keep the method handle's loader allocator | ||
| // alive here in case the shared generic method is inside a collectible | ||
| // ALC? |
There was a problem hiding this comment.
@davidwrighton @VSadov Any idea? Is it sufficient to keep the LoaderAllocator associated with the MethodTable* generic context alive here? I see that MethodDesc::GetLoaderAllocator gets it via GetLoaderModule()->GetLoaderAllocator(), but MethodDesc::GetLoaderModule() seemingly may not always match GetMethodTable()->GetLoaderModule() based on this code:
There was a problem hiding this comment.
Is it sufficient to keep the LoaderAllocator associated with the MethodTable* generic context alive here?
I do not think so. You can have void MyMethod<T>() instantiated as void MyMethod<UnloadableType>. The MethodTable won't know anything about the UnloadableType.
There was a problem hiding this comment.
Ignore my comment. I have missed that this path is for the MethodTable* generic context case.
It is sufficient to keep the LoaderAllocator associated with the MethodTable* generic context alive here.
|
PTAL @VSadov @davidwrighton |
jakobbotsch
changed the title
LoaderAllocator objects alive from continuationsLoaderAllocator objects alive from continuations
| GenTree* dataSizeNode = m_comp->gtNewIconNode((ssize_t)dataSize, TYP_I_IMPL); | ||
| // If VM requests that we report the method handle, or if we have a shared generic context method handle | ||
| // that is live here, then we need to call a different helper. | ||
| GenTree* methodHandleArg = nullptr; |
There was a problem hiding this comment.
If I read this correctly we will use an alternative continuation allocators unconditionally, if we have generic contexts and allocator will sort it out if there is an object to hold on to.
I guess we cannot know statically if we will have loader allocator or not. Not a big deal, I think.
There was a problem hiding this comment.
Yes, I think all shared generics need to go through these new helpers. We cannot know if the generic context was instantiated with an unloadable type during JIT.
For the continuationsNeedMethodHandle case I assume the loader allocator will always be non-null (since it's only set when the ALC is collectible).
|
Should we have a test, at least for the We are probably not ready to stress-test this for GC holes, but maybe for code coverage - to make sure the alternative code paths are functional. |
The shared-generic test does exercise the two new helpers. I can add one that uses collectible ALCs to also exercise the collectible ALC one. |
|
I pushed a test and verified that it fails without the change (with AV from trying to resume in unloaded code) and succeeds with it. |
jakobbotsch
merged commit 4daa761
into
dotnet:feature/async2-experiment
Add logic to make sure we keep the exposed loader allocator object alive when a runtime async function suspends. This adds two new JIT helpers
CORINFO_HELP_ALLOC_CONTINUATION_CLASSandCORINFO_HELP_ALLOC_CONTINUATION_METHODthat store theLoaderAllocatorassociated with a class or method into the allocated continuation before returning it back to the JIT.There are a few cases where the JIT has to call the new helpers:
LoaderAllocatorassociated with the method being compiled aliveMethodDesc*as the generic context must keep theLoaderAllocatorassocated with thatMethodDesc*aliveMethodTable*as the generic context must similarly keep that one alive.Fix #2759