Skip to content

Optimize DispatchProxy generated code #47134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Optimize DispatchProxy generated code #47134

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 66 additions & 124 deletions ...libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ namespace System.Reflection
//
internal static class DispatchProxyGenerator
{
// Generated proxies have a private Action field that all generated methods
// invoke. It is the first field in the class and the first ctor parameter.
private const int InvokeActionFieldAndCtorParameterIndex = 0;
// Generated proxies have a private MethodInfo[] field that generated methods use to get the corresponding MethodInfo.
// It is the first field in the class and the first ctor parameter.
private const int MethodInfosFieldAndCtorParameterIndex = 0;

// Proxies are requested for a pair of types: base type and interface type.
// The generated proxy will subclass the given base type and implement the interface type.
Expand All @@ -57,9 +57,11 @@ internal static class DispatchProxyGenerator
// This approach is used to prevent regenerating identical proxy types for identical T/Proxy pairs,
// which would ultimately be a more expensive leak.
// Proxy instances are not cached. Their lifetime is entirely owned by the caller of DispatchProxy.Create.
private static readonly Dictionary<Type, Dictionary<Type, Type>> s_baseTypeAndInterfaceToGeneratedProxyType = new Dictionary<Type, Dictionary<Type, Type>>();
private static readonly Dictionary<Type, Dictionary<Type, GeneratedTypeInfo>> s_baseTypeAndInterfaceToGeneratedProxyType = new Dictionary<Type, Dictionary<Type, GeneratedTypeInfo>>();
private static readonly ProxyAssembly s_proxyAssembly = new ProxyAssembly();
private static readonly MethodInfo s_dispatchProxyInvokeMethod = typeof(DispatchProxy).GetTypeInfo().GetDeclaredMethod("Invoke")!;
private static readonly MethodInfo s_getTypeFromHandleMethod = typeof(Type).GetRuntimeMethod("GetTypeFromHandle", new Type[] { typeof(RuntimeTypeHandle) })!;
private static readonly MethodInfo s_makeGenericMethodMethod = typeof(MethodInfo).GetMethod("MakeGenericMethod", new Type[] { typeof(Type[]) })!;
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure reflection-accessing reflection surface area results in a trimming warning. Invoking reflection APIs with parameters that illink didn't see is not safe.

Cc @vitek-karas I filed dotnet/linker#1764.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

In this case, the usage is "safe" since the type arguments all come from the user's code. So any requirements on the types will be seen by the linker there.

Here was the original suppression:

runtime/src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs

Lines 149 to 157 in 9f1c913

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2060:MakeGenericMethod",
Justification = "MakeGenericMethod is safe here because the user code invoking the generic method will reference " +
"the GenericTypes being used, which will guarantee the requirements of the generic method.")]
private static void Invoke(object?[] args)
{
PackedArgs packed = new PackedArgs(args);
MethodBase method = s_proxyAssembly.ResolveMethodToken(packed.DeclaringType, packed.MethodToken);
if (method.IsGenericMethodDefinition)
method = ((MethodInfo)method).MakeGenericMethod(packed.GenericTypes!);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, maybe DispatchProxy is not trimming safe after all.

interface IFoo
{
    [return: DynamicallyAccessedMembers(PublicConstructors)]
    Type GetTheType();
}

IFoo someFoo = GetItFromDispatchProxy();
Activator.CreateInstance(someFoo.GetTheType());

If we make a DispatchProxy for this, who is going to enforce that the Type returned from the implementation of GetTheType matches the requirements?

There was a discussion whether we need to mark the interface methods as reflected up here: #46715 (comment)

I think we have to. And if we do that and once dotnet/linker#1764 is fixed, this will generate a warning because we in fact cannot statically enforce that this will hold.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, maybe DispatchProxy is not trimming safe after all.

I meant to say "not trimming safe for all possible interfaces". This would only warn if the interface has the DynamicallyAccessed annotations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will probably have to take this into account - see dotnet/linker#1607 (comment). Originally I though that return value annotations are not affected, since they apply to the implementation not the caller, but with TypeBuilder I can provide implementation to annotated method without linker seeing it that way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichalStrehovsky that would mean that we need to mark the interface type in the proxy with All as well - so that linker would "mark as reflection" all of the interfaces and all the members on them. With the fix in dotnet/linker#1764 that would lead to generating a warning in the above case.
If we don't mark it as All, we would not be able to catch this. Ideally we would need some annotation like "consider all used methods on this type/interface as accessed via reflection" - but that's way too specific annotation to create. So All is probably the best for now.

So to answer @eerhardt question:

  • We need an issue tracking that this scenario is actually property handled (all up) - not sure if that should live in runtime or linker
  • Annotate the interface type in dispatch proxy with All - I know we originally reverted that, but given the above, I don't see a better way currently. It's probably not a big size concern anyway, since it will only increase the size of the proxy itself, which is typically very small (each method's implementation is pretty trivial).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotate the interface type in dispatch proxy with All

This is the part I don't understand. If a method on an interface is marked as DynamicallyAccessed and isn't being used, why would we need to warn about it? In the scenario above, GetTheType() is called. So I guess I don't understand how annotating the interface as All should change anything here.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotating it as All will trigger a warning at the callsite to DispatchProxy's Create once dotnet/linker#1764 is addressed and the interface has any methods that have annotations. The purpose is just to trigger the warning when creating a DispatchProxy for an annotated interface. It's not about preserving stuff.

If it's not annotated and we're suppressing the warning within the method body there's no place that would warn in such situation.

Linker needs to see that the interface method is potentially reflection-accessed.

Basically, the problem is that there's a suppression on the call to TypeBuilder.AddInterfaceImplementation within the DispatchProxy code. This will never be safe to suppress if the interface is user-provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @MichalStrehovsky for the explanation. Another way to look at it is: Each annotation has two sides to it. The code which assigns value to "it" and the code which uses the value from "it". Linker needs to validate both sides. In the case of an annotation on a return value, the code which assigns the value is the method's body, and the code which uses the value is the caller. In this case with dispatch proxy and the interface method, the caller is known (as you mention there will be call to this method somewhere in the app's code), but the method's body is not known to linker - it will be created at runtime via TypeBuilder. So linker can only validate one side - which is not enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a sample I used to wrap my head around this:

using System;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

IFooer f = DispatchProxy.Create<IFooer, MyFooer>();
Activator.CreateInstance(f.MakeFoo());

class MyFooer : DispatchProxy
{
    protected override object Invoke(MethodInfo targetMethod, object[] args)
    {
        return typeof(Unused);
    }
}

class Unused
{
}

interface IFooer
{
    [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
    Type MakeFoo();
}

Linker will not keep the ctor of Unused but there's also no warning. So I was thinking "what's the spot that should have warned". The spot was the TypeBuilder.AddInterfaceImplementation that we suppressed - linker needs to see that we reflection-access the interface methods. Propagating that annotation out to the public API surface will make sure there is a warning for this case (once the above mentioned bug is fixed), because indeed illink cannot prove that this will work.

Suppressions need a lot of scrutiny :(.


// Returns a new instance of a proxy the derives from 'baseType' and implements 'interfaceType'
internal static object CreateProxyInstance(
Expand All @@ -69,24 +71,23 @@ internal static object CreateProxyInstance(
Debug.Assert(baseType != null);
Debug.Assert(interfaceType != null);

Type proxiedType = GetProxyType(baseType, interfaceType);
return Activator.CreateInstance(proxiedType, (Action<object[]>)DispatchProxyGenerator.Invoke)!;
GeneratedTypeInfo proxiedType = GetProxyType(baseType, interfaceType);
return Activator.CreateInstance(proxiedType.GeneratedType, new object[] { proxiedType.MethodInfos })!;
}

[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
private static Type GetProxyType(
private static GeneratedTypeInfo GetProxyType(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type baseType,
Type interfaceType)
{
lock (s_baseTypeAndInterfaceToGeneratedProxyType)
{
if (!s_baseTypeAndInterfaceToGeneratedProxyType.TryGetValue(baseType, out Dictionary<Type, Type>? interfaceToProxy))
if (!s_baseTypeAndInterfaceToGeneratedProxyType.TryGetValue(baseType, out Dictionary<Type, GeneratedTypeInfo>? interfaceToProxy))
{
interfaceToProxy = new Dictionary<Type, Type>();
interfaceToProxy = new Dictionary<Type, GeneratedTypeInfo>();
s_baseTypeAndInterfaceToGeneratedProxyType[baseType] = interfaceToProxy;
}

if (!interfaceToProxy.TryGetValue(interfaceType, out Type? generatedProxy))
if (!interfaceToProxy.TryGetValue(interfaceType, out GeneratedTypeInfo? generatedProxy))
{
generatedProxy = GenerateProxyType(baseType, interfaceType);
interfaceToProxy[interfaceType] = generatedProxy;
Expand All @@ -97,8 +98,7 @@ private static Type GetProxyType(
}

// Unconditionally generates a new proxy type derived from 'baseType' and implements 'interfaceType'
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
private static Type GenerateProxyType(
private static GeneratedTypeInfo GenerateProxyType(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type baseType,
Type interfaceType)
{
Expand Down Expand Up @@ -139,59 +139,23 @@ private static Type GenerateProxyType(

pb.AddInterfaceImpl(interfaceType);

Type generatedProxyType = pb.CreateType();
GeneratedTypeInfo generatedProxyType = pb.CreateType();
return generatedProxyType;
}

// All generated proxy methods call this static helper method to dispatch.
// Its job is to unpack the arguments and the 'this' instance and to dispatch directly
// to the (abstract) DispatchProxy.Invoke() method.
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2060:MakeGenericMethod",
Justification = "MakeGenericMethod is safe here because the user code invoking the generic method will reference " +
"the GenericTypes being used, which will guarantee the requirements of the generic method.")]
private static void Invoke(object?[] args)
private class GeneratedTypeInfo
{
PackedArgs packed = new PackedArgs(args);
MethodBase method = s_proxyAssembly.ResolveMethodToken(packed.DeclaringType, packed.MethodToken);
if (method.IsGenericMethodDefinition)
method = ((MethodInfo)method).MakeGenericMethod(packed.GenericTypes!);

// Call (protected method) DispatchProxy.Invoke()
try
{
Debug.Assert(s_dispatchProxyInvokeMethod != null);
object? returnValue = s_dispatchProxyInvokeMethod!.Invoke(packed.DispatchProxy,
new object?[] { method, packed.Args });
packed.ReturnValue = returnValue;
}
catch (TargetInvocationException tie)
public GeneratedTypeInfo(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type generatedType,
MethodInfo[] methodInfos)
{
Debug.Assert(tie.InnerException != null);
ExceptionDispatchInfo.Capture(tie.InnerException).Throw();
GeneratedType = generatedType;
MethodInfos = methodInfos;
}
}

private class PackedArgs
{
internal const int DispatchProxyPosition = 0;
internal const int DeclaringTypePosition = 1;
internal const int MethodTokenPosition = 2;
internal const int ArgsPosition = 3;
internal const int GenericTypesPosition = 4;
internal const int ReturnValuePosition = 5;

internal static readonly Type[] PackedTypes = new Type[] { typeof(object), typeof(Type), typeof(int), typeof(object[]), typeof(Type[]), typeof(object) };

private readonly object?[] _args;
internal PackedArgs() : this(new object[PackedTypes.Length]) { }
internal PackedArgs(object?[] args) { _args = args; }

internal DispatchProxy? DispatchProxy { get { return (DispatchProxy?)_args[DispatchProxyPosition]; } }
internal Type? DeclaringType { get { return (Type?)_args[DeclaringTypePosition]; } }
internal int MethodToken { get { return (int)_args[MethodTokenPosition]!; } }
internal object[]? Args { get { return (object[]?)_args[ArgsPosition]; } }
internal Type[]? GenericTypes { get { return (Type[]?)_args[GenericTypesPosition]; } }
internal object? ReturnValue { /*get { return args[ReturnValuePosition]; }*/ set { _args[ReturnValuePosition] = value; } }
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
public Type GeneratedType { get; }
public MethodInfo[] MethodInfos { get; }
}

private class ProxyAssembly
Expand All @@ -200,10 +164,6 @@ private class ProxyAssembly
private readonly ModuleBuilder _mb;
private int _typeId;

// Maintain a MethodBase-->int, int-->MethodBase mapping to permit generated code
// to pass methods by token
private readonly Dictionary<MethodBase, int> _methodToToken = new Dictionary<MethodBase, int>();
private readonly List<MethodBase> _methodsByToken = new List<MethodBase>();
private readonly HashSet<string?> _ignoresAccessAssemblyNames = new HashSet<string?>();
private ConstructorInfo? _ignoresAccessChecksToAttributeConstructor;

Expand Down Expand Up @@ -268,36 +228,16 @@ internal void EnsureTypeIsVisible(Type type)
}
}
}

internal void GetTokenForMethod(MethodBase method, out Type type, out int token)
{
Debug.Assert(method.DeclaringType != null);
type = method.DeclaringType!;
token = 0;
if (!_methodToToken.TryGetValue(method, out token))
{
_methodsByToken.Add(method);
token = _methodsByToken.Count - 1;
_methodToToken[method] = token;
}
}

internal MethodBase ResolveMethodToken(Type? type, int token)
{
Debug.Assert(token >= 0 && token < _methodsByToken.Count);
return _methodsByToken[token];
}
}

private class ProxyBuilder
{
private static readonly MethodInfo s_delegateInvoke = typeof(Action<object[]>).GetMethod("Invoke")!;

private readonly ProxyAssembly _assembly;
private readonly TypeBuilder _tb;
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
private readonly Type _proxyBaseType;
private readonly List<FieldBuilder> _fields;
private readonly List<MethodInfo> _methodInfos;

internal ProxyBuilder(
ProxyAssembly assembly,
Expand All @@ -309,7 +249,9 @@ internal ProxyBuilder(
_proxyBaseType = proxyBaseType;

_fields = new List<FieldBuilder>();
_fields.Add(tb.DefineField("invoke", typeof(Action<object[]>), FieldAttributes.Private));
_fields.Add(tb.DefineField("_methodInfos", typeof(MethodInfo[]), FieldAttributes.Private));

_methodInfos = new List<MethodInfo>();
}

private void Complete()
Expand Down Expand Up @@ -341,11 +283,10 @@ private void Complete()
il.Emit(OpCodes.Ret);
}

[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
internal Type CreateType()
internal GeneratedTypeInfo CreateType()
{
this.Complete();
return _tb.CreateType()!;
return new GeneratedTypeInfo(_tb.CreateType()!, _methodInfos.ToArray());
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2067:UnrecognizedReflectionPattern",
Expand Down Expand Up @@ -388,7 +329,9 @@ internal void AddInterfaceImpl(Type iface)
if (!mi.IsVirtual || mi.IsFinal)
continue;

MethodBuilder mdb = AddMethodImpl(mi);
int methodInfoIndex = _methodInfos.Count;
_methodInfos.Add(mi);
MethodBuilder mdb = AddMethodImpl(mi, methodInfoIndex);
if (propertyMap.TryGetValue(mi, out PropertyAccessorInfo? associatedProperty))
{
if (MethodInfoEqualityComparer.Instance.Equals(associatedProperty.InterfaceGetMethod, mi))
Expand Down Expand Up @@ -446,7 +389,7 @@ internal void AddInterfaceImpl(Type iface)
}
}

private MethodBuilder AddMethodImpl(MethodInfo mi)
private MethodBuilder AddMethodImpl(MethodInfo mi, int methodInfoIndex)
{
ParameterInfo[] parameters = mi.GetParameters();
Type[] paramTypes = ParamTypes(parameters, false);
Expand Down Expand Up @@ -487,54 +430,52 @@ private MethodBuilder AddMethodImpl(MethodInfo mi)
}
}

// object[] packed = new object[PackedArgs.PackedTypes.Length];
GenericArray<object> packedArr = new GenericArray<object>(il, PackedArgs.PackedTypes.Length);

// packed[PackedArgs.DispatchProxyPosition] = this;
packedArr.BeginSet(PackedArgs.DispatchProxyPosition);
// MethodInfo methodInfo = _methodInfos[methodInfoIndex];
LocalBuilder methodInfoLocal = il.DeclareLocal(typeof(MethodInfo));
il.Emit(OpCodes.Ldarg_0);
packedArr.EndSet(typeof(DispatchProxy));

// packed[PackedArgs.DeclaringTypePosition] = typeof(iface);
MethodInfo Type_GetTypeFromHandle = typeof(Type).GetRuntimeMethod("GetTypeFromHandle", new Type[] { typeof(RuntimeTypeHandle) })!;
_assembly.GetTokenForMethod(mi, out Type declaringType, out int methodToken);
packedArr.BeginSet(PackedArgs.DeclaringTypePosition);
il.Emit(OpCodes.Ldtoken, declaringType);
il.Emit(OpCodes.Call, Type_GetTypeFromHandle);
packedArr.EndSet(typeof(object));

// packed[PackedArgs.MethodTokenPosition] = iface method token;
packedArr.BeginSet(PackedArgs.MethodTokenPosition);
il.Emit(OpCodes.Ldc_I4, methodToken);
packedArr.EndSet(typeof(int));

// packed[PackedArgs.ArgsPosition] = args;
packedArr.BeginSet(PackedArgs.ArgsPosition);
argsArr.Load();
packedArr.EndSet(typeof(object[]));
il.Emit(OpCodes.Ldfld, _fields[MethodInfosFieldAndCtorParameterIndex]); // MethodInfo[] _methodInfos
il.Emit(OpCodes.Ldc_I4, methodInfoIndex);
il.Emit(OpCodes.Ldelem_Ref);
il.Emit(OpCodes.Stloc, methodInfoLocal);

// packed[PackedArgs.GenericTypesPosition] = mi.GetGenericArguments();
if (mi.ContainsGenericParameters)
{
packedArr.BeginSet(PackedArgs.GenericTypesPosition);
// methodInfo = methodInfo.MakeGenericMethod(mi.GetGenericArguments());
il.Emit(OpCodes.Ldloc, methodInfoLocal);

Type[] genericTypes = mi.GetGenericArguments();
GenericArray<Type> typeArr = new GenericArray<Type>(il, genericTypes.Length);
for (int i = 0; i < genericTypes.Length; ++i)
{
typeArr.BeginSet(i);
il.Emit(OpCodes.Ldtoken, genericTypes[i]);
il.Emit(OpCodes.Call, Type_GetTypeFromHandle);
il.Emit(OpCodes.Call, s_getTypeFromHandleMethod);
typeArr.EndSet(typeof(Type));
}
typeArr.Load();
packedArr.EndSet(typeof(Type[]));

il.Emit(OpCodes.Callvirt, s_makeGenericMethodMethod);
il.Emit(OpCodes.Stloc, methodInfoLocal);
}

// Call static DispatchProxyHelper.Invoke(object[])
// object result = this.Invoke(methodInfo, args);
LocalBuilder? resultLocal = mi.ReturnType != typeof(void) ?
il.DeclareLocal(typeof(object)) :
null;
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldfld, _fields[InvokeActionFieldAndCtorParameterIndex]); // delegate
packedArr.Load();
il.Emit(OpCodes.Call, s_delegateInvoke);
il.Emit(OpCodes.Ldloc, methodInfoLocal);
argsArr.Load();
il.Emit(OpCodes.Callvirt, s_dispatchProxyInvokeMethod);

if (resultLocal != null)
{
il.Emit(OpCodes.Stloc, resultLocal);
}
else
{
// drop the result for void methods
il.Emit(OpCodes.Pop);
}

for (int i = 0; i < parameters.Length; i++)
{
Expand All @@ -546,9 +487,10 @@ private MethodBuilder AddMethodImpl(MethodInfo mi)
}
}

if (mi.ReturnType != typeof(void))
if (resultLocal != null)
{
packedArr.Get(PackedArgs.ReturnValuePosition);
// return (mi.ReturnType)result;
il.Emit(OpCodes.Ldloc, resultLocal);
Convert(il, typeof(object), mi.ReturnType, false);
}

Expand Down