Lower reflection overhead

src/Adapter/MSTest.TestAdapter/BannedSymbols.txt

@@ -0,0 +1,2 @@
+T:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.NotCachedReflectHelper; This is allowed only for tests.
+M:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.ReflectHelper.#ctor; This is allowed only for tests.

src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs

@@ -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);
+            bool typeIgnored = ReflectHelper.IsNonDerivedAttributeDefined<IgnoreAttribute>(type, false);
 
             if (warningsFromTypeEnumerator != null)
             {
@@ -263,6 +263,11 @@ private bool DynamicDataAttached(IDictionary<string, object?> sourceLevelParamet
             return false;
         }
 
+        if (test.TestMethod.DataType != DynamicDataType.ITestDataSource)
+        {
+            return false;
+        }
+
         // NOTE: From this place we don't have any path that would let the user write a message on the TestContext and we don't do
         // anything with what would be printed anyway so we can simply use a simple StringWriter.
         using var writer = new StringWriter();
@@ -275,11 +280,11 @@ private bool DynamicDataAttached(IDictionary<string, object?> sourceLevelParamet
     private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMethodInfo testMethodInfo, List<UnitTestElement> tests)
     {
         MethodInfo methodInfo = testMethodInfo.MethodInfo;
-        IEnumerable<FrameworkITestDataSource>? testDataSources = ReflectHelper.GetAttributes<Attribute>(methodInfo, false)?.OfType<FrameworkITestDataSource>();
-        if (testDataSources == null || !testDataSources.Any())
-        {
-            return false;
-        }
+
+        // We don't have a special method to filter attributes that are not derived from Attribute, so we take all
+        // attributes and filter them. We don't have to care if there is one, because this method is only entered when
+        // there is at least one (we determine this in TypeEnumerator.GetTestFromMethod.
+        IEnumerable<FrameworkITestDataSource>? testDataSources = ReflectHelper.Instance.GetDerivedAttributes<Attribute>(methodInfo, inherit: false).OfType<FrameworkITestDataSource>();
 
         try
         {

src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs

@@ -1,9 +1,10 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System.Globalization;
 using System.Reflection;
 
+using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution;
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Extensions;
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -48,8 +49,14 @@ internal TestMethodValidator(ReflectHelper reflectHelper, bool discoverInternals
     /// <returns> Return true if a method is a valid test method. </returns>
     internal virtual bool IsValidTestMethod(MethodInfo testMethodInfo, Type type, ICollection<string> warnings)
     {
-        if (!_reflectHelper.IsAttributeDefined<TestMethodAttribute>(testMethodInfo, false)
-            && !_reflectHelper.HasAttributeDerivedFrom<TestMethodAttribute>(testMethodInfo, false))
+        // PERF: Contrary to my intuition, we are doing caching reflection here, meaning we will cache every method info in the
+        // assembly, this is because when we discover and run we will repeatedly inspect all the methods in the assembly, and this
+        // gives us a better performance.
+        // It would be possible to use non-caching reflection here if we knew that we are only doing discovery that won't be followed by run,
+        // but the difference is quite small, and we don't expect a huge amount of non-test methods in the assembly.
+        //
+        // Also skip all methods coming from object, because they cannot be tests.
+        if (testMethodInfo.DeclaringType == typeof(object) || !_reflectHelper.IsDerivedAttributeDefined<TestMethodAttribute>(testMethodInfo, inherit: false))
         {
             return false;
         }

src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs

@@ -73,6 +73,8 @@ internal Collection<UnitTestElement> GetTests(ICollection<string> warnings)
         var tests = new Collection<UnitTestElement>();
 
         // Test class is already valid. Verify methods.
+        // PERF: GetRuntimeMethods is used here to get all methods, including non-public, and static methods.
+        // if we rely on analyzers to identify all invalid methods on build, we can change this to fit the current settings.
         foreach (MethodInfo method in _type.GetRuntimeMethods())
         {
             bool isMethodDeclaredInTestTypeAssembly = _reflectHelper.IsMethodDeclaredInSameAssemblyAsType(method, _type);
@@ -147,14 +149,35 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT
                     method.DeclaringType.GetTypeInfo().Assembly);
         }
 
+        // PERF: AssemblyEnumerator would later try to figure if this is a data driven test method
+        // by creating a TestMethodInfo. This is really expensive for discovery, and should be done only
+        // when we are a data driven test.
+        // Instead we check if we have any data driven attribute here, and skip creating TestMethod info
+        // in AssemblyEnumerator.
+        //
+        // The foreach below could move to ReflectHelper, but this is only place where we want to check if
+        // an attribute implements something different than an Attribute derived class. So instead we just
+        // grab all attributes, and do the check ourselves.
+        bool isDataDriven = false;
+        foreach (Attribute attribute in _reflectHelper.GetDerivedAttributes<Attribute>(method, inherit: true))
+        {
+            if (AttributeComparer.IsDerived<ITestDataSource>(attribute))
+            {
+                isDataDriven = true;
+                break;
+            }
+        }
+
+        testMethod.DataType = isDataDriven ? DynamicDataType.ITestDataSource : DynamicDataType.None;
+
         var testElement = new UnitTestElement(testMethod)
         {
             // Get compiler generated type name for async test method (either void returning or task returning).
             AsyncTypeName = method.GetAsyncTypeName(),
-            TestCategory = _reflectHelper.GetCategories(method, _type),
+            TestCategory = _reflectHelper.GetTestCategories(method, _type),
             DoNotParallelize = _reflectHelper.IsDoNotParallelizeSet(method, _type),
             Priority = _reflectHelper.GetPriority(method),
-            Ignored = _reflectHelper.IsAttributeDefined<IgnoreAttribute>(method, false),
+            Ignored = _reflectHelper.IsNonDerivedAttributeDefined<IgnoreAttribute>(method, inherit: false),
             DeploymentItems = PlatformServiceProvider.Instance.TestDeployment.GetDeploymentItems(method, _type, warnings),
         };
 
@@ -174,29 +197,29 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT
 
         testElement.Traits = traits.ToArray();
 
-        if (_reflectHelper.GetCustomAttribute<CssIterationAttribute>(method) is CssIterationAttribute cssIteration)
+        if (_reflectHelper.GetFirstDerivedAttributeOrDefault<CssIterationAttribute>(method, inherit: true) is CssIterationAttribute cssIteration)
         {
             testElement.CssIteration = cssIteration.CssIteration;
         }
 
-        if (_reflectHelper.GetCustomAttribute<CssProjectStructureAttribute>(method) is CssProjectStructureAttribute cssProjectStructure)
+        if (_reflectHelper.GetFirstDerivedAttributeOrDefault<CssProjectStructureAttribute>(method, inherit: true) is CssProjectStructureAttribute cssProjectStructure)
         {
             testElement.CssProjectStructure = cssProjectStructure.CssProjectStructure;
         }
 
-        if (_reflectHelper.GetCustomAttribute<DescriptionAttribute>(method) is DescriptionAttribute descriptionAttribute)
+        if (_reflectHelper.GetFirstDerivedAttributeOrDefault<DescriptionAttribute>(method, inherit: true) is DescriptionAttribute descriptionAttribute)
         {
             testElement.Description = descriptionAttribute.Description;
         }
 
-        WorkItemAttribute[] workItemAttributes = _reflectHelper.GetCustomAttributes<WorkItemAttribute>(method);
+        WorkItemAttribute[] workItemAttributes = _reflectHelper.GetDerivedAttributes<WorkItemAttribute>(method, inherit: true).ToArray();
         if (workItemAttributes.Length != 0)
         {
             testElement.WorkItemIds = workItemAttributes.Select(x => x.Id.ToString(CultureInfo.InvariantCulture)).ToArray();
         }
 
         // get DisplayName from TestMethodAttribute (or any inherited attribute)
-        TestMethodAttribute? testMethodAttribute = _reflectHelper.GetCustomAttribute<TestMethodAttribute>(method);
+        TestMethodAttribute? testMethodAttribute = _reflectHelper.GetFirstDerivedAttributeOrDefault<TestMethodAttribute>(method, inherit: true);
         testElement.DisplayName = testMethodAttribute?.DisplayName ?? method.Name;
 
         return testElement;

src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs

@@ -51,9 +51,12 @@ internal virtual bool IsValidTestClass(Type type, ICollection<string> warnings)
     {
         TypeInfo typeInfo = type.GetTypeInfo();
 
-        if (!typeInfo.IsClass
-            || (!_reflectHelper.IsAttributeDefined<TestClassAttribute>(typeInfo, false)
-            && !_reflectHelper.HasAttributeDerivedFrom<TestClassAttribute>(typeInfo, false)))
+        // PERF: Contrary to my intuition, we are doing caching reflection here, meaning we will cache every class info in the
+        // assembly, this is because when we discover and run we will repeatedly inspect all the types in the assembly, and this
+        // gives us a better performance.
+        // It would be possible to use non-caching reflection here if we knew that we are only doing discovery that won't be followed by run,
+        // but the difference is quite small, and we don't expect a huge amount of non-test classes in the assembly.
+        if (!typeInfo.IsClass || !_reflectHelper.IsDerivedAttributeDefined<TestClassAttribute>(typeInfo, inherit: false))
         {
             return false;
         }

src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs

@@ -1,6 +1,7 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System.Diagnostics;
 using System.Globalization;
 
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery;
@@ -58,7 +59,17 @@ internal UnitTestDiscoverer()
         ITestCaseDiscoverySink discoverySink,
         IDiscoveryContext? discoveryContext)
     {
+        TimeSpan getTests;
+        var sw = Stopwatch.StartNew();
+#if NET6_0_OR_GREATER
+        long beforeDiscovery = GC.GetTotalAllocatedBytes();
+#endif
         ICollection<UnitTestElement>? testElements = _assemblyEnumeratorWrapper.GetTests(source, discoveryContext?.RunSettings, out ICollection<string>? warnings);
+        getTests = sw.Elapsed;
+#if NET6_0_OR_GREATER
+        long afterDiscovery = GC.GetTotalAllocatedBytes();
+#endif
+        sw.Restart();
 
         bool treatDiscoveryWarningsAsErrors = MSTestSettings.CurrentSettings.TreatDiscoveryWarningsAsErrors;
 
@@ -85,6 +96,14 @@ internal UnitTestDiscoverer()
             source);
 
         SendTestCases(source, testElements, discoverySink, discoveryContext, logger);
+        TimeSpan sendOverhead = sw.Elapsed;
+#if NET6_0_OR_GREATER
+        long afterSend = GC.GetTotalAllocatedBytes();
+#endif
+        Console.WriteLine($"discovered: {testElements.Count} tests in {getTests.TotalMilliseconds} ms, sent them in {sendOverhead.TotalMilliseconds} ms, total: {sendOverhead.TotalMilliseconds + getTests.TotalMilliseconds} <<<");
+#if NET6_0_OR_GREATER
+        Console.WriteLine($"discovered: discovery alloc: {(afterDiscovery - beforeDiscovery) / (1024 * 1024)} MB send alloc: {(afterSend - afterDiscovery) / (1024 * 1024)} MB");
+#endif
     }
 
     internal void SendTestCases(string source, IEnumerable<UnitTestElement> testElements, ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext, IMessageLogger logger)

src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs

@@ -0,0 +1,33 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution;
+
+internal static class KnownNonTestMethods
+{
+    private static readonly string[] MethodNames = [
+        nameof(Equals),
+        nameof(GetHashCode),
+        nameof(GetType),
+        nameof(ReferenceEquals),
+        nameof(ToString)
+        ];
+
+    public static bool Contains(string methodName)
+    {
+        if (methodName == "Equals")
+        {
+            Console.WriteLine($"Known non test methods called with: {methodName}");
+            Console.WriteLine(Environment.StackTrace);
+        }
+        foreach (string method in MethodNames)
+        {
+            if (string.Equals(method, methodName, StringComparison.Ordinal))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+}

src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System.Diagnostics;
@@ -82,11 +82,16 @@ public class TestMethodInfo : ITestMethod
     /// </summary>
     internal TestMethodOptions TestMethodOptions { get; }
 
-    public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.GetCustomAttributes(TestMethod, inherit) as Attribute[];
+    // This is allowed because we don't know the usage.
+#pragma warning disable RS0030 // Do not use banned APIs
+    [Obsolete("don't use, but cannot remove")]
+    public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.Instance.NotCachedReflectHelper.GetCustomAttributesNotCached(TestMethod, inherit) as Attribute[];
+#pragma warning restore RS0030 // Do not use banned APIs
 
+    [Obsolete("don't use, but cannot remove")]
     public TAttributeType[] GetAttributes<TAttributeType>(bool inherit)
         where TAttributeType : Attribute
-        => ReflectHelper.GetAttributes<TAttributeType>(TestMethod, inherit)
+        => ReflectHelper.Instance.NotCachedReflectHelper.GetCustomAttributesNotCached<TAttributeType>(TestMethod, inherit)
         ?? Array.Empty<TAttributeType>();
 
     /// <summary>