From d11af0561ca6f99cda4d03877c3d2a8a38d5016c Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 7 Aug 2020 10:48:03 -0700 Subject: [PATCH 1/7] Use DynamicDependency to prevent struct GetEnumerator from being removed by linker. --- .../src/System/Diagnostics/Activity.cs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index 23834958a4f4cb..8a203375dcc7d0 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -76,10 +76,20 @@ public partial class Activity : IDisposable private byte _w3CIdFlags; + /* + * Note: + * DynamicDependency is used here to prevent the linker from removing the struct GetEnumerator on the internal types. + * Some customers use this GetEnumerator dynamically to avoid allocations. + * See: https://github.com/dotnet/runtime/pull/40362 + */ + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(TagsLinkedList))] private TagsLinkedList? _tags; - private LinkedList>? _baggage; + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(LinkedList))] private LinkedList? _links; + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(LinkedList))] private LinkedList? _events; + + private LinkedList>? _baggage; private ConcurrentDictionary? _customProperties; private string? _displayName; @@ -255,9 +265,6 @@ public string? RootId /// public IEnumerable> TagObjects { -#if ALLOW_PARTIALLY_TRUSTED_CALLERS - [System.Security.SecuritySafeCriticalAttribute] -#endif get => _tags ?? s_emptyTagObjects; } From 526817dcd8f78663da1ce3ed444ee9b132657d08 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 7 Aug 2020 11:07:00 -0700 Subject: [PATCH 2/7] Code review. --- .../src/System/Diagnostics/Activity.cs | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index 8a203375dcc7d0..7a524170478af4 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -76,20 +76,11 @@ public partial class Activity : IDisposable private byte _w3CIdFlags; - /* - * Note: - * DynamicDependency is used here to prevent the linker from removing the struct GetEnumerator on the internal types. - * Some customers use this GetEnumerator dynamically to avoid allocations. - * See: https://github.com/dotnet/runtime/pull/40362 - */ - [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(TagsLinkedList))] + private TagsLinkedList? _tags; - [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(LinkedList))] + private LinkedList>? _baggage; private LinkedList? _links; - [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(LinkedList))] private LinkedList? _events; - - private LinkedList>? _baggage; private ConcurrentDictionary? _customProperties; private string? _displayName; @@ -1280,8 +1271,8 @@ public void Add(T value) } public Enumerator GetEnumerator() => new Enumerator(_first); - IEnumerator IEnumerable.GetEnumerator() => new Enumerator(_first); - IEnumerator IEnumerable.GetEnumerator() => new Enumerator(_first); + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); } private class TagsLinkedList : IEnumerable> @@ -1387,8 +1378,8 @@ public void Set(KeyValuePair value) } public Enumerator> GetEnumerator() => new Enumerator>(_first); - IEnumerator> IEnumerable>.GetEnumerator() => new Enumerator>(_first); - IEnumerator IEnumerable.GetEnumerator() => new Enumerator>(_first); + IEnumerator> IEnumerable>.GetEnumerator() => GetEnumerator(); + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); public IEnumerable> EnumerateStringValues() { From 0af8fa964e50304d45f215e7611cd41301cc68f3 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 7 Aug 2020 11:07:55 -0700 Subject: [PATCH 3/7] Removed empty line. --- .../src/System/Diagnostics/Activity.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index 7a524170478af4..0f93c77138d309 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -76,7 +76,6 @@ public partial class Activity : IDisposable private byte _w3CIdFlags; - private TagsLinkedList? _tags; private LinkedList>? _baggage; private LinkedList? _links; From 5d0262c56376a4bd295175c9b91f36269857db72 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 7 Aug 2020 12:09:32 -0700 Subject: [PATCH 4/7] Code review. --- .../src/System/Diagnostics/Activity.cs | 7 +++ .../tests/ActivityTests.cs | 48 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index 0f93c77138d309..aabcd80ef368f3 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -1269,6 +1269,7 @@ public void Add(T value) } } + // Note: Some customers use this GetEnumerator dynamically to avoid allocations. public Enumerator GetEnumerator() => new Enumerator(_first); IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); @@ -1376,6 +1377,7 @@ public void Set(KeyValuePair value) } } + // Note: Some customers use this GetEnumerator dynamically to avoid allocations. public Enumerator> GetEnumerator() => new Enumerator>(_first); IEnumerator> IEnumerable>.GetEnumerator() => GetEnumerator(); IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); @@ -1396,6 +1398,11 @@ public void Set(KeyValuePair value) } } + /* + * Note: + * Some customers use this Enumerator dynamically to avoid allocations. + * See: https://github.com/dotnet/runtime/pull/40362 + */ private struct Enumerator : IEnumerator { private readonly LinkedListNode? _head; diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs index a085d3103bdbd7..17fc5dcf8f8b04 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs @@ -1508,6 +1508,54 @@ public void TestInsertingFirstTag(string key, object value, bool add, int result Assert.Equal(resultCount, a.TagObjects.Count()); } + [Fact] + public void StructEnumerator_TagsLinkedList() + { + /* + * This test verifies the presence of the struct Enumerator on TagsLinkedList used by customers dynamically to avoid allocations. + */ + Activity a = new Activity("TestActivity"); + a.AddTag("Tag1", true); + + IEnumerable> enumerable = a.TagObjects; + + bool foundGetEnumerator = false; + foreach (MethodInfo method in enumerable.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.NonPublic)) + { + if(method.Name == "GetEnumerator" && !method.ReturnType.IsInterface && method.ReturnType.IsValueType) + { + foundGetEnumerator = true; + break; + } + } + + Assert.True(foundGetEnumerator); + } + + [Fact] + public void StructEnumerator_GenericLinkedList() + { + /* + * This test verifies the presence of the struct Enumerator on LinkedList used by customers dynamically to avoid allocations. + */ + Activity a = new Activity("TestActivity"); + a.AddEvent(new ActivityEvent()); + + IEnumerable enumerable = a.Events; + + bool foundGetEnumerator = false; + foreach (MethodInfo method in enumerable.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.NonPublic)) + { + if (method.Name == "GetEnumerator" && !method.ReturnType.IsInterface && method.ReturnType.IsValueType) + { + foundGetEnumerator = true; + break; + } + } + + Assert.True(foundGetEnumerator); + } + public void Dispose() { Activity.Current = null; From dd4b9018953febbaccf5083db48a85e1425c071a Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 7 Aug 2020 12:18:53 -0700 Subject: [PATCH 5/7] Update comment style. --- .../src/System/Diagnostics/Activity.cs | 6 +----- .../tests/ActivityTests.cs | 10 ++++------ 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index aabcd80ef368f3..2cf1a6254c0236 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -1398,11 +1398,7 @@ public void Set(KeyValuePair value) } } - /* - * Note: - * Some customers use this Enumerator dynamically to avoid allocations. - * See: https://github.com/dotnet/runtime/pull/40362 - */ + // Note: Some customers use this Enumerator dynamically to avoid allocations. private struct Enumerator : IEnumerator { private readonly LinkedListNode? _head; diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs index 17fc5dcf8f8b04..eb60ca3cd8d257 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs @@ -1511,9 +1511,8 @@ public void TestInsertingFirstTag(string key, object value, bool add, int result [Fact] public void StructEnumerator_TagsLinkedList() { - /* - * This test verifies the presence of the struct Enumerator on TagsLinkedList used by customers dynamically to avoid allocations. - */ + // Note: This test verifies the presence of the struct Enumerator on TagsLinkedList used by customers dynamically to avoid allocations. + Activity a = new Activity("TestActivity"); a.AddTag("Tag1", true); @@ -1535,9 +1534,8 @@ public void StructEnumerator_TagsLinkedList() [Fact] public void StructEnumerator_GenericLinkedList() { - /* - * This test verifies the presence of the struct Enumerator on LinkedList used by customers dynamically to avoid allocations. - */ + // Note: This test verifies the presence of the struct Enumerator on LinkedList used by customers dynamically to avoid allocations. + Activity a = new Activity("TestActivity"); a.AddEvent(new ActivityEvent()); From b9a4865d2acf3b7f1441c7331ab98e3fe1e5328e Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 7 Aug 2020 12:22:47 -0700 Subject: [PATCH 6/7] Code review. --- .../tests/ActivityTests.cs | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs index eb60ca3cd8d257..cdd6f15f5682c6 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs @@ -1518,17 +1518,11 @@ public void StructEnumerator_TagsLinkedList() IEnumerable> enumerable = a.TagObjects; - bool foundGetEnumerator = false; - foreach (MethodInfo method in enumerable.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.NonPublic)) - { - if(method.Name == "GetEnumerator" && !method.ReturnType.IsInterface && method.ReturnType.IsValueType) - { - foundGetEnumerator = true; - break; - } - } + MethodInfo method = enumerable.GetType().GetMethod("GetEnumerator", BindingFlags.Instance | BindingFlags.Public); - Assert.True(foundGetEnumerator); + Assert.NotNull(method); + Assert.False(method.ReturnType.IsInterface); + Assert.True(method.ReturnType.IsValueType); } [Fact] @@ -1541,17 +1535,11 @@ public void StructEnumerator_GenericLinkedList() IEnumerable enumerable = a.Events; - bool foundGetEnumerator = false; - foreach (MethodInfo method in enumerable.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.NonPublic)) - { - if (method.Name == "GetEnumerator" && !method.ReturnType.IsInterface && method.ReturnType.IsValueType) - { - foundGetEnumerator = true; - break; - } - } + MethodInfo method = enumerable.GetType().GetMethod("GetEnumerator", BindingFlags.Instance | BindingFlags.Public); - Assert.True(foundGetEnumerator); + Assert.NotNull(method); + Assert.False(method.ReturnType.IsInterface); + Assert.True(method.ReturnType.IsValueType); } public void Dispose() From 0fdba3fc315da6ea2abf87753f73cdb4dc0232d8 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Sat, 8 Aug 2020 12:04:37 -0700 Subject: [PATCH 7/7] Drop support for reset on struct Enumerator. --- .../src/System/Diagnostics/Activity.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index 2cf1a6254c0236..5addc00ee27dd6 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -1401,13 +1401,12 @@ public void Set(KeyValuePair value) // Note: Some customers use this Enumerator dynamically to avoid allocations. private struct Enumerator : IEnumerator { - private readonly LinkedListNode? _head; private LinkedListNode? _nextNode; [AllowNull, MaybeNull] private T _currentItem; public Enumerator(LinkedListNode? head) { - _nextNode = _head = head; + _nextNode = head; _currentItem = default; } @@ -1428,7 +1427,7 @@ public bool MoveNext() return true; } - public void Reset() => _nextNode = _head; + public void Reset() => throw new NotSupportedException(); public void Dispose() {