From 394e5e3c9d1f53263251e6f9ad6f337b53bf6468 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 4 Dec 2020 23:41:38 +0000 Subject: [PATCH 1/4] Use faster single attribute lookup for Eventsource --- .../System/Diagnostics/Tracing/EventSource.cs | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs index 1e67a98ed310f6..db79ad0e7ad93e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs @@ -2748,6 +2748,32 @@ private unsafe void SendManifest(byte[]? rawManifest) #endif // FEATURE_MANAGED_ETW } + // Helper to deal with the fact that the type we are reflecting over might be loaded in the ReflectionOnly context. + // When that is the case, we have the build the custom assemblies on a member by hand. + internal static bool IsCustomAttributeDefinedHelper( + MemberInfo member, + Type attributeType, + EventManifestOptions flags = EventManifestOptions.None) + { + // AllowEventSourceOverride is an option that allows either Microsoft.Diagnostics.Tracing or + // System.Diagnostics.Tracing EventSource to be considered valid. This should not mattter anywhere but in Microsoft.Diagnostics.Tracing (nuget package). + if (!member.Module.Assembly.ReflectionOnly && (flags & EventManifestOptions.AllowEventSourceOverride) == 0) + { + // Let the runtime to the work for us, since we can execute code in this context. + return member.IsDefined(attributeType); + } + + foreach (CustomAttributeData data in CustomAttributeData.GetCustomAttributes(member)) + { + if (AttributeTypeNamesMatch(attributeType, data.Constructor.ReflectedType!)) + { + return true; + } + } + + return false; + } + // Helper to deal with the fact that the type we are reflecting over might be loaded in the ReflectionOnly context. // When that is the case, we have the build the custom assemblies on a member by hand. internal static Attribute? GetCustomAttributeHelper( @@ -2763,13 +2789,7 @@ private unsafe void SendManifest(byte[]? rawManifest) if (!member.Module.Assembly.ReflectionOnly && (flags & EventManifestOptions.AllowEventSourceOverride) == 0) { // Let the runtime to the work for us, since we can execute code in this context. - Attribute? firstAttribute = null; - foreach (object attribute in member.GetCustomAttributes(attributeType, false)) - { - firstAttribute = (Attribute)attribute; - break; - } - return firstAttribute; + return member.GetCustomAttribute(attributeType, inherit: false); } foreach (CustomAttributeData data in CustomAttributeData.GetCustomAttributes(member)) @@ -3019,7 +3039,7 @@ private static bool AttributeTypeNamesMatch(Type attributeType, Type reflectedAt } // If we explicitly mark the method as not being an event, then honor that. - if (GetCustomAttributeHelper(method, typeof(NonEventAttribute), flags) != null) + if (IsCustomAttributeDefinedHelper(method, typeof(NonEventAttribute), flags)) continue; defaultEventAttribute = new EventAttribute(eventId); @@ -5475,7 +5495,7 @@ static FieldInfo[] GetEnumFields(Type localEnumType) sb.AppendLine(" "); foreach (Type enumType in mapsTab.Values) { - bool isbitmap = EventSource.GetCustomAttributeHelper(enumType, typeof(FlagsAttribute), flags) != null; + bool isbitmap = EventSource.IsCustomAttributeDefinedHelper(enumType, typeof(FlagsAttribute), flags); string mapKind = isbitmap ? "bitMap" : "valueMap"; sb.Append(" <").Append(mapKind).Append(" name=\"").Append(enumType.Name).AppendLine("\">"); From 52bbce3337c5bba135a7f2d9f7619ff49fb791e2 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Tue, 8 Dec 2020 17:00:45 +0000 Subject: [PATCH 2/4] Update src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs Co-authored-by: Eric Erhardt --- .../src/System/Diagnostics/Tracing/EventSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs index db79ad0e7ad93e..5c5aa43cffd579 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs @@ -2760,7 +2760,7 @@ internal static bool IsCustomAttributeDefinedHelper( if (!member.Module.Assembly.ReflectionOnly && (flags & EventManifestOptions.AllowEventSourceOverride) == 0) { // Let the runtime to the work for us, since we can execute code in this context. - return member.IsDefined(attributeType); + return member.IsDefined(attributeType, inherit: false); } foreach (CustomAttributeData data in CustomAttributeData.GetCustomAttributes(member)) From 9790c20d32d0a874b74b7b90f48ced7a8498de08 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Tue, 8 Dec 2020 17:06:21 +0000 Subject: [PATCH 3/4] Add debug assert for type --- .../src/System/Diagnostics/Tracing/EventSource.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs index 5c5aa43cffd579..985f1e77500980 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs @@ -2784,6 +2784,7 @@ internal static bool IsCustomAttributeDefinedHelper( Type attributeType, EventManifestOptions flags = EventManifestOptions.None) { + Debug.Assert(attributeType == typeof(EventAttribute) || attributeType == typeof(EventSourceAttribute)); // AllowEventSourceOverride is an option that allows either Microsoft.Diagnostics.Tracing or // System.Diagnostics.Tracing EventSource to be considered valid. This should not mattter anywhere but in Microsoft.Diagnostics.Tracing (nuget package). if (!member.Module.Assembly.ReflectionOnly && (flags & EventManifestOptions.AllowEventSourceOverride) == 0) From c47a557f1f696f037aaddab2a89f8b3686f40ccf Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 9 Jan 2021 21:01:52 +0000 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Stephen Toub --- .../src/System/Diagnostics/Tracing/EventSource.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs index 985f1e77500980..c6913ba2e5208b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs @@ -2749,7 +2749,7 @@ private unsafe void SendManifest(byte[]? rawManifest) } // Helper to deal with the fact that the type we are reflecting over might be loaded in the ReflectionOnly context. - // When that is the case, we have the build the custom assemblies on a member by hand. + // When that is the case, we have to build the custom assemblies on a member by hand. internal static bool IsCustomAttributeDefinedHelper( MemberInfo member, Type attributeType, @@ -2759,7 +2759,7 @@ internal static bool IsCustomAttributeDefinedHelper( // System.Diagnostics.Tracing EventSource to be considered valid. This should not mattter anywhere but in Microsoft.Diagnostics.Tracing (nuget package). if (!member.Module.Assembly.ReflectionOnly && (flags & EventManifestOptions.AllowEventSourceOverride) == 0) { - // Let the runtime to the work for us, since we can execute code in this context. + // Let the runtime do the work for us, since we can execute code in this context. return member.IsDefined(attributeType, inherit: false); } @@ -2789,7 +2789,7 @@ internal static bool IsCustomAttributeDefinedHelper( // System.Diagnostics.Tracing EventSource to be considered valid. This should not mattter anywhere but in Microsoft.Diagnostics.Tracing (nuget package). if (!member.Module.Assembly.ReflectionOnly && (flags & EventManifestOptions.AllowEventSourceOverride) == 0) { - // Let the runtime to the work for us, since we can execute code in this context. + // Let the runtime do the work for us, since we can execute code in this context. return member.GetCustomAttribute(attributeType, inherit: false); }