Skip to content
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.

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

[Discussion] MetadataLoadContext & runtime reflection #27800

Closed
ilmax opened this issue Nov 2, 2018 · 8 comments
Closed

[Discussion] MetadataLoadContext & runtime reflection #27800

ilmax opened this issue Nov 2, 2018 · 8 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection
Milestone

Comments

@ilmax
Copy link

ilmax commented Nov 2, 2018

Question/Idea
I'm following TypeLoader implementation and I would like to know if there will be a way to quickly get a runtime reflection counterpart (e.g. get a RuntimePropertyInfo from a RoProperty) form the most used (IMHO) reflection types e.g. PropertyInfo, FieldInfo, MethodInfo, TypeInfo.

I poked around a bit and looks like for some types is kind of easy to get the runtime counterpart using MetadataToken e.g.

public static FieldInfo ToRuntimeType(this RoField field)
    => typeof(Program).Assembly.GetModule(field.Module.Name).ResolveField(field.MetadataToken);

public static Type ToRuntimeType(this Type type)
    => typeof(Program).Assembly.GetModule(type.Module.Name).ResolveType(type.MetadataToken);

Motivation

Reflection is used a lot, and it's known to be a bit slow, so I created a couple of benchmark to compare reflection and typeloader implementation to compare them.

My benchmark are iterating over all types defined in mscorlib doing different level of inspection (only inspecting types, inspecting all properties for each type, all fields and all methods) below you can find the benchmark result

Benchamrk code is here I'm using Linqpad to run it and I'm using System.Reflection.TypeLoader from corefx-lab version 0.1.0-preview2-181102-1,

BenchmarkDotNet=v0.11.2, OS=Windows 10.0.17763.1 (1809/October2018Update/Redstone5)

Intel Core i7-8750H CPU 2.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores

  [Host]     : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.3190.0
  DefaultJob : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.3190.0


                          Method |     Mean |     Error |    StdDev |      Min |        Max | Ratio | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |

-------------------------------- |---------:|----------:|----------:|---------:|-----------:|------:|------------:|------------:|------------:|--------------------:|
 ReflectionDiscoverAllTypes      | 999.4 us |  12.40 us |  11.60 us | 982.0 us | 1,025.2 us |  1.00 |           - |           - |           - |             7.28 KB |
 TypeLoaderDiscoverAllTypes      | 697.3 us |  12.40 us |  10.99 us | 682.1 us |   717.6 us |  0.70 |      6.8359 |      6.8359 |           - |             34.2 KB |
-------------------------------- |---------:|----------:|----------:|---------:|-----------:|------:|------------:|------------:|------------:|--------------------:|
 ReflectionDiscoverAllFields     | 1.794 ms | 0.0176 ms | 0.0165 ms | 1.767 ms |   1.823 ms |  1.00 |     25.3906 |           - |           - |           125.18 KB |
 TypeLoaderDiscoverAllFields     | 1.165 ms | 0.0231 ms | 0.0266 ms | 1.125 ms |   1.207 ms |  0.65 |     23.4375 |     23.4375 |           - |           110.36 KB |
-------------------------------- |---------:|----------:|----------:|---------:|-----------:|------:|------------:|------------:|------------:|--------------------:|
 ReflectionDiscoverAllProperties | 2.537 ms | 0.0273 ms | 0.0242 ms | 2.494 ms |   2.588 ms |  1.00 |     39.0625 |           - |           - |           197.53 KB |
 TypeLoaderDiscoverAllProperties | 1.164 ms | 0.0224 ms | 0.0299 ms | 1.102 ms |   1.209 ms |  0.46 |     21.4844 |     21.4844 |           - |           106.49 KB |
-------------------------------- |---------:|----------:|----------:|---------:|-----------:|------:|------------:|------------:|------------:|--------------------:|
 ReflectionDiscoverAllMethods    | 68.75 ms | 0.5679 ms | 0.5312 ms | 67.86 ms |   69.81 ms |  1.00 |   3375.0000 |           - |           - |            15.48 MB |
 TypeLoaderDiscoverAllMethods    | 10.29 ms | 0.1984 ms | 0.1949 ms | 10.02 ms |   10.69 ms |  0.15 |    375.0000 |           - |           - |             1.75 MB |

The benchmark shows that typeloader is way faster when we start inspecting properties/fields/methods. The downside of typeloader is that we cannot use it's reflection types to e.g. invoke a method or get a field value because typeloader is supposed to be a replacement (or alternative to) Assembly.ReflectionOnlyLoad according to this, but a lot of libraries (I'm thinking about EF Core, MVC just to name few) are doing extensive reflection work on loaded assemblies that can be mostly satisfied by typeloader and after all the discovery is done, they require the full fledged reflection to be able to perform runtime invocation, so the ability to swap e.g. a typeloader's methodInfo to a runtimemethodinfo would be cool and would make it possible to use typeloader for the discovery phase, making it a lot faster and less memory intensive (see the allocated memory for the method discovery above) and then swap to runtime reflection to perform all the runtime work required (method invocation, get/set properties and so on).

I made few quick test and I was able to get a RuntimeField and RuntimeType easilly, I'm having some trouble with RuntimePropertyInfo, I will keep poking around here.

What do you think about this?

@karelz
Copy link
Member

karelz commented Nov 2, 2018

cc @jkotas

@jkotas
Copy link
Member

jkotas commented Nov 2, 2018

Yes, we know that mapping the reflection-only types back to the regular ones is likely to be common. Adding helper APIs to make the mapping easier sounds reasonable to me.

cc @steveharter

@ilmax
Copy link
Author

ilmax commented Nov 2, 2018

@jkotas I'm not able to find a way to get a RuntimePropertyInfo given a MetadataToken and a Module, not sure why module exposes few Resolve* methods, but a ResolveProperty is missing and ResolveMember for property throws. Is there any specific reason or can we add

public MethodBase ResolveProperty(int metadataToken) => ResolveProperty(metadataToken, null, null);
public virtual MethodBase ResolveProperty(int metadataToken, Type[] genericTypeArguments, Type[] genericMethodArguments) { throw NotImplemented.ByDesign; }

in class Module and implement them in RuntimeModule?

So far the conversion from reflection-only property to runtime PropertyInfo is the only one not available, if this is available, then this api addition can move forward.

@jkotas
Copy link
Member

jkotas commented Nov 3, 2018

The mapping via token works only if the runtime assembly and MetadataLoadContext assembly are exactly the same. If they are not exactly the same (e.g. the MetadataLoadContext operates on reference assemblies), the mapping has to be done by name.

I think these APIs should transparently handle both cases. The mapping via token for properties then becomes just missing minor performance optimization that can be worked on independently.

@ilmax
Copy link
Author

ilmax commented Nov 7, 2018

@jkotas you mean something similar to this?

Type ToRuntimeType(this RoType type, Module module)
{
	if (module != null) 
	{
		return module.ResolveType(type.MetadataToken);
	}
	
	// Slow path
	return Type.GetType(type.AssemblyQualifiedName, true, false);
}

Field ToRuntimeField(this RoField field, Module module)
{
	if (module != null) 
	{
		return module.ResolveField(field.MetadataToken);
	}
	
	// Slow path
	BindingFlags flags;
	if (field.IsPublic) 
	{
		flags = BindingFlags.Public;
	}
	else
	{
		flags = BindingFlags.NonPublic;
	}

	if (field.IsStatic) 
	{
		flags |= BindingFlags.Static;
	}
	else
	{
		flags |= BindingFlags.Instance;
	}
	
	return Type.GetType(type.AssemblyQualifiedName, true, false).GetField(field.Name, flags);
}

@jkotas
Copy link
Member

jkotas commented Nov 7, 2018

Yes, something like this. The fast path may need to be more robust - what would guarantee that the type or field is actually from the exact module passed in?

@ilmax
Copy link
Author

ilmax commented Nov 7, 2018

@jkotas Yes I know, I will try to come up with something for this, we also have another issue, how can we expose this (extension?) method because all the Ro* reflection types are internal, so ATM I don't know where to expose these methods.

Maybe we can add a new class that exposes a Project() extension method (I'm looking at the project used in corefx-lab unit tests) to allow going from runtime reflection types to typeloader ones (do they have an official fancy name 😄?) and expose the other way around (what am I actually proposing here) in the same class, so we can have a guarantee that the module passed in is the correct one (because it will be done by this new class) and also overcome the internal issue.
There are some disadvantages to this approach, like hold few collections over the type and also we will hide the usage of TypeLoader (behind the project method) thus being somehow less flexible, but this is an opt-in feature so if a user need more flexibility it can do it on it's own.

What do you think about this one?

P.S. To make the mapping via runtime token work for properties, we need to look into exposing a ResolveProperty method on the Module class because so far I wasn't able to find a way to get a RuntimePropertyInfo given a module and a token.
Are there any reasons/limitations on not exposing this one or it's just because it wasn't necessary?

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@steveharter steveharter changed the title [Discussion] TypeLoader & runtime reflection [Discussion] MetadataLoadContext & runtime reflection Mar 6, 2020
@steveharter
Copy link
Member

Going from ROTypes to runtime types can be done in a robust way (the same assembly must already be loaded) but this particular feature appears to be for performance and per discussion with @GrabYourPitchforks this won't meet the bar for 5.0 especially since we are planning to improve runtime reflection performance for key scenarios via #31895.

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Mar 6, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection
Projects
None yet
Development

No branches or pull requests

6 participants