Lower reflection overhead #2839
Conversation
| @@ -219,7 +219,7 @@ internal virtual TypeEnumerator GetTypeEnumerator(Type type, string assemblyFile | |||
| typeFullName = type.FullName; | |||
| TypeEnumerator testTypeEnumerator = GetTypeEnumerator(type, assemblyFileName, discoverInternals, testIdGenerationStrategy); | |||
| ICollection<UnitTestElement>? unitTestCases = testTypeEnumerator.Enumerate(out ICollection<string>? warningsFromTypeEnumerator); | |||
| bool typeIgnored = ReflectHelper.IsAttributeDefined<IgnoreAttribute>(type, false); | |||
There was a problem hiding this comment.
IsAttributeDefined checks attribute entry by fully qualified assembly name. So it is a non-derived attribute (it needs to be exactly the same type).
| @@ -174,29 +190,29 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT | |||
|
|
|||
| testElement.Traits = traits.ToArray(); | |||
|
|
|||
| if (_reflectHelper.GetCustomAttribute<CssIterationAttribute>(method) is CssIterationAttribute cssIteration) | |||
There was a problem hiding this comment.
GetCustomAttribute checks for a derived attribute (e.g. MyTestClass when we look for TestClass), and inherits (inspects parent types).
All our attributes allow inheritance, even though we don't always check for it.
Almost none of our attributes allow multiple, just testCategory, testProperty, and workItem.
So getting first without checking that there is only Single is okay, because there never won't be more, unless you are using the attributes above.
| @@ -1,6 +1,7 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
This helps me measure, I will revert, or we can log it?
|
|
||
| // Update class initialize/cleanup method | ||
| UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, false, ref initAndCleanupMethods); | ||
| if (methodInfo.IsPublic && !methodInfo.IsStatic) |
There was a problem hiding this comment.
This is done in the other place as well, but seems inconsistent with the rest of the code, where we inspect even shapes of methods that are obviously incorrect, and throw when they have incorrect shape.
So remove in both places?
| @@ -426,16 +441,15 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) | |||
| { | |||
| assemblyInfo.AssemblyInitializeMethod = methodInfo; | |||
|
|
|||
| if (_reflectionHelper.IsAttributeDefined<TimeoutAttribute>(methodInfo, false)) | |||
There was a problem hiding this comment.
Yum yum, this code checks for the presence of timeout 3 times per check.
| //if (!methodInfo.IsStatic) | ||
| //{ | ||
| // return false; | ||
| //} |
There was a problem hiding this comment.
This is another similar inconcistency.
| @@ -300,7 +300,7 @@ private class ClassCleanupManager | |||
| g => new HashSet<string>(g.Select(t => t.TestMethod.UniqueName)))); | |||
| _lifecycleFromMsTest = lifecycleFromMsTest; | |||
| _lifecycleFromAssembly = lifecycleFromAssembly; | |||
| _reflectHelper = reflectHelper ?? new ReflectHelper(); | |||
There was a problem hiding this comment.
The code above already does not accept null, and we don't want to duplicate our cache.
| return false; | ||
| } | ||
| // TODO: redesign this, probably change this to GetTimeout? so we don't have to do this weird dance? | ||
| timeoutAttribute ??= ReflectHelper.Instance.GetSingleNonDerivedAttributeOrDefault<TimeoutAttribute>(method, inherit: false, nullOnMultiple: true); |
There was a problem hiding this comment.
Or better ideas to redesign? Before the change it was grabbing the attributes 3 times to check:
- if the attribute is defined
- then again in validation method
- then again to grab the timeout
| @@ -125,7 +122,7 @@ internal static bool IsValidReturnType(this MethodInfo method) | |||
| /// <returns>Compiler generated type name for given async test method..</returns> | |||
| internal static string? GetAsyncTypeName(this MethodInfo method) | |||
| { | |||
| AsyncStateMachineAttribute? asyncStateMachineAttribute = ReflectHelper.GetCustomAttributes<AsyncStateMachineAttribute>(method, false).FirstOrDefault(); | |||
There was a problem hiding this comment.
This method does not allow multiple or inherit. So we replace with getting first, non derived, without inheritance.
This PR simplifies the way we access reflection in discovery and run. By improving the attribute cache and using it whenever possible.
It also simplifies ReflectHelper code, by removing dependencies between methods, and giving them more descriptive names. To make it much easier to reason about the current and desired behavior.
I am keeping the existing non-consistent behavior, to avoid breaking changes.
GetCustomAttribute => GetFirstDerivedAttributeOrDefault + inherit: true - when used with attributes that allow only one instance
GetCustomAttributes -> GetDerivedAttributes + inherit: true - for attributes that allow multiple.
Before
Initial measurements, on SDK 3.3.1.
Discovery of 10k empty tests:
discovered: 10000 tests in 1992,1448 ms, sent them in 289,5684 ms, total: 2281,7132
discovered: 10000 tests in 899,1829 ms, sent them in 212,4562 ms, total: 1111,6391
discovered: 10000 tests in 552,0988 ms, sent them in 321,4334 ms, total: 873,5322
Run 10k empty tests:
After
Discover 10 empty tests:
discovered: 10000 tests in 1219,4222 ms, sent them in 227,5277 ms, total: 1446,9499 <<<
2088
discovered: 10000 tests in 324,3993 ms, sent them in 193,5436 ms, total: 517,9429 <<<
1024
discovered: 10000 tests in 259,1427 ms, sent them in 291,6613 ms, total: 550,804 <<<
discovered: discovery alloc: 50 MB send alloc: 72 MB
971
Run 10k empty tests: