Allow caching IDynamicInterfaceCastable results#117646
Allow caching IDynamicInterfaceCastable results#117646MichalStrehovsky merged 3 commits intodotnet:mainfrom
Conversation
Contributes to dotnet#107999. We want _some_ caching. It's unclear what the right caching is (the caching on CoreCLR VM side is [wild](dotnet#107999 (comment))), but given that we sometimes might need to purge caches to avoid turning them into memory leaks, I think the only conclusion is that whoever implements things like: ```csharp class Dyn : IDynamicInterfaceCastable { public Type InterfaceType { get; init; } public Type ImplType { get; init; } public RuntimeTypeHandle GetInterfaceImplementation(RuntimeTypeHandle interfaceType) => interfaceType.Equals(InterfaceType.TypeHandle) ? ImplType.TypeHandle : throw new Exception(); public bool IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented) => interfaceType.Equals(InterfaceType.TypeHandle) ? true : (throwIfNotImplemented ? throw new InvalidCastException() : false); } ``` Should feel bad, and we don't support their use case.
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the dispatch cache logic to remove the special-case for IDynamicInterfaceCastable, aiming to allow caching of its results in Native AOT.
- Removed the branch that bypassed cache updates for
IDynamicInterfaceCastable - Simplified the
if (pTargetCode != IntPtr.Zero)block
Comments suppressed due to low confidence (1)
src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/CachedInterfaceDispatch.cs:30
- By removing the
else return pTargetCodebranch, calls forIDynamicInterfaceCastableobjects now fall through to the fail-fast path instead of returning the resolved target. Restore a directreturn pTargetCode;for the dynamic-castable case or refactor so both paths correctly return.
if (pTargetCode != IntPtr.Zero)
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
|
Do we need to add some tests? |
Wouldn't it require building a lot of infrastructure so that we can control when/how the cache gets flushed to get anything reliable? The CoreCLR caching behavior is baffling and making a native AOT specific test for this really doesn't feel like worth the effort. |
|
So the I honestly think that the caching here is user-hostile, whoever disagrees, just try to root cause the failing test in a debugger. |
Resolves #107999.
The caching on CoreCLR VM side is wild. The native AOT cache is per-callsite. So we will have observable differences in how the cache works. I assume anyone who comes to us with "My
Gizmodoesn't work for more than one type" whereGizmois defined as:will be sent away, although I can't find docs that we could point them to saying this is illegal. The CoreCLR VM implementation that sends things through a cache doesn't seem to allow that, and neither will native AOT.
Cc @dotnet/ilc-contrib @AaronRobinsonMSFT @jkoritzinsky