Improve algorithm for reflection

[Evangelink]

Some parts of the code are using a reflection cache and some others are not. Also, we are splitting logic in many places when it seems that doing a single loop and saving results in a cache to be reused elsewhere in the code would be easier and more performant.

AB#2050309

[nohwnd]

I was researching this, and we can improve the speed by 10% or more by doing this, (this cuts the time time in reflection to half) by grabbing the collection of attributes once from assembly, class, and method. And pass them down where needed. This has impact especially on paths where we grab attributes with inheritance.

The implementation would replace the many calls to:

method.GetCustomAttributes<TestMethodAttribute>()
method.GetCustomAttributes<PriorityAttribute>()
method.GetCustomAttributes<TestCategoryAttribute>(inherit: true)

with

IReadOnlyCollection<Attribute> methodAttributes = method.GetCustomAttributes();
IReadOnlyCollection<Attribute> inheritedMethodAttributes = method.GetCustomAttributes(inherit: true);

methodAttributes.GetSingleOrNull<TestMethodAttribute>();
etc.

And similarly for Assembly, and Class, where we just need to grab the collection once and pass it down where needed.

Also calls retrieve the attribute and compare to null should be avoided, and replaced either with the api above, or with IsDefined.

More measurements need to be made, e.g. around allocation, but most of the changes are (crudely) implemented here: https://github.com/nohwnd/testfx/tree/discovery-optimize-and-break

[testplatform-bot]

✅ Successfully linked to Azure Boards work item(s):

• User Story 2050309

[nohwnd]

I did some measurements and benchmarks and this is my plan:

• unify usage of reflection

Split the usage of reflection into 2 categories, cached and non-cached. All the places that touch all types to see if the class or method is a test will use reflection "directly".

All other places that are populating more info about a known test class or a test will use cached reflector helper. Or type cache, which uses cached reflector helper.

This will avoid lot of the time and cost of looking up attributes.

• simplify ReflectHelper
  Reflect helper is super convoluted and it is hard to see if we are doing things the best way and using the cache.

• use cache everywhere in reflect helper

• cache inherited and non-inherited attributes separately

• use isAttributeDefined where appropriate.

is Attribute defined is allocation free and is cheap when used without inheritance. it is surprisingly more expensive then getCustomAttributes when used with inheritance

[Evangelink]

Split the usage of reflection into 2 categories, cached and non-cached. All the places that touch all types to see if the class or method is a test will use reflection "directly".

All other places that are populating more info about a known test class or a test will use cached reflector helper. Or type cache, which uses cached reflector helper.

This will avoid lot of the time and cost of looking up attributes.

Just to be sure I understand, the goal is to first to a "real" reflection call that will populate the cache and all subsequent calls would only read from the cache without really doing any call, right?

[nohwnd]

Yes. Right now ReflectorHelper is a mashup of direct reflection calls and cached reflection calls, and a jungle of calls in between the internal and public methods in that class, so I have no idea if the call I am making is cheap or expensive.

I want to untangle that to make the methods more independent, so we can know what they do exactly.

And split the api so it is easy to see where we are calling into data that are cached, and where we call reflection directly without cache.

Reflect helper will of course also internally call the reflection api to get the data, but then it will cache them.