From 14c7e590fc8568530ead38cc34de7444ed346f15 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 11 Jun 2019 11:28:40 -0700 Subject: [PATCH] Remove allocation for empty Activity.Baggage/Tags (dotnet/corefx#38389) Accessing Activity.Baggage/Tags allocates an enumerable, even when there is no baggage or tags. This fast-paths the empty case. Commit migrated from https://github.com/dotnet/corefx/commit/ec93eb4e8a0b225c1bfca093396328e6ba1fe9d2 --- .../src/System/Diagnostics/Activity.cs | 48 ++++++++++++-- .../tests/ActivityTests.cs | 75 +++++++++++++++++++--- 2 files changed, 106 insertions(+), 17 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 a6551b8..61b692c 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -30,6 +30,8 @@ namespace System.Diagnostics /// public partial class Activity { + private static readonly IEnumerable> s_emptyBaggageTags = new KeyValuePair[0]; // Array.Empty() doesn't exist in all configurations + /// /// An operation name is a COARSEST name that is useful grouping/filtering. /// The name is typically a compile-time constant. Names of Rest APIs are @@ -151,8 +153,20 @@ namespace System.Diagnostics { get { - for (var tags = _tags; tags != null; tags = tags.Next) - yield return tags.keyValue; + KeyValueListNode tags = _tags; + return tags != null ? + Iterate(tags) : + s_emptyBaggageTags; + + static IEnumerable> Iterate(KeyValueListNode tags) + { + do + { + yield return tags.keyValue; + tags = tags.Next; + } + while (tags != null); + } } } @@ -169,8 +183,28 @@ namespace System.Diagnostics get { for (Activity activity = this; activity != null; activity = activity.Parent) - for (var baggage = activity._baggage; baggage != null; baggage = baggage.Next) - yield return baggage.keyValue; + { + if (activity._baggage != null) + { + return Iterate(activity); + } + } + + return s_emptyBaggageTags; + + static IEnumerable> Iterate(Activity activity) + { + do + { + for (KeyValueListNode baggage = activity._baggage; baggage != null; baggage = baggage.Next) + { + yield return baggage.keyValue; + } + + activity = activity.Parent; + } + while (activity != null); + } } } @@ -180,7 +214,7 @@ namespace System.Diagnostics /// public string GetBaggageItem(string key) { - foreach (var keyValue in Baggage) + foreach (KeyValuePair keyValue in Baggage) if (key == keyValue.Key) return keyValue.Value; return null; @@ -357,7 +391,7 @@ namespace System.Diagnostics { if (_parentId == null && !_parentSpanIdSet) { - var parent = Current; + Activity parent = Current; if (parent != null) { // The parent change should not form a loop. We are actually guaranteed this because @@ -442,7 +476,7 @@ namespace System.Diagnostics { for (Activity activity = this; activity != null; activity = activity.Parent) { - var val = activity._traceState; + string val = activity._traceState; if (val != null) return val; } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs index c7b2366..eed9134 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs @@ -39,13 +39,63 @@ namespace System.Diagnostics.Tests public void Baggage() { var activity = new Activity("activity"); + + // Baggage starts off empty Assert.Null(activity.GetBaggageItem("some baggage")); + Assert.Empty(activity.Baggage); + + const string Key = "some baggage"; + const string Value = "value"; - Assert.Equal(activity, activity.AddBaggage("some baggage", "value")); - Assert.Equal("value", activity.GetBaggageItem("some baggage")); + // Add items, get them individually and via an enumerable + for (int i = 0; i < 3; i++) + { + Assert.Equal(activity, activity.AddBaggage(Key + i, Value + i)); + Assert.Equal(Value + i, activity.GetBaggageItem(Key + i)); + Assert.Equal(i + 1, activity.Baggage.Count()); + for (int j = 0; j < i; j++) + { + Assert.Contains(new KeyValuePair(Key + j, Value + j), activity.Baggage); + } + } - var baggage = activity.Baggage.ToList(); - Assert.Equal(1, baggage.Count); + // Now test baggage via child activities + activity.Start(); + try + { + // Start a child + var anotherActivity = new Activity("another activity"); + anotherActivity.Start(); + try + { + // Its parent should match. + Assert.Same(activity, anotherActivity.Parent); + + // All of the parent's baggage should be available via the child. + for (int i = 0; i < 3; i++) + { + Assert.Equal(Value + i, anotherActivity.GetBaggageItem(Key + i)); + Assert.Contains(new KeyValuePair(Key + i, Value + i), anotherActivity.Baggage); + } + + // And we should be able to add additional baggage to the child, see that baggage, and still see the parent's. + Assert.Same(anotherActivity, anotherActivity.AddBaggage("hello", "world")); + Assert.Equal("world", anotherActivity.GetBaggageItem("hello")); + Assert.Equal(4, anotherActivity.Baggage.Count()); + Assert.Equal(new KeyValuePair("hello", "world"), anotherActivity.Baggage.First()); + Assert.Equal(new KeyValuePair(Key + 2, Value + 2), anotherActivity.Baggage.Skip(1).First()); + Assert.Equal(new KeyValuePair(Key + 1, Value + 1), anotherActivity.Baggage.Skip(2).First()); + Assert.Equal(new KeyValuePair(Key + 0, Value + 0), anotherActivity.Baggage.Skip(3).First()); + } + finally + { + anotherActivity.Stop(); + } + } + finally + { + activity.Stop(); + } } /// @@ -55,13 +105,18 @@ namespace System.Diagnostics.Tests public void Tags() { var activity = new Activity("activity"); + Assert.Empty(activity.Tags); - Assert.Equal(activity, activity.AddTag("some tag", "value")); - - var tags = activity.Tags.ToList(); - Assert.Equal(1, tags.Count); - Assert.Equal(tags[0].Key, "some tag"); - Assert.Equal(tags[0].Value, "value"); + const string Key = "some tag"; + const string Value = "value"; + for (int i = 0; i < 3; i++) + { + Assert.Equal(activity, activity.AddTag(Key + i, Value + i)); + List> tags = activity.Tags.ToList(); + Assert.Equal(i + 1, tags.Count); + Assert.Equal(tags[tags.Count - i - 1].Key, Key + i); + Assert.Equal(tags[tags.Count - i - 1].Value, Value + i); + } } /// -- 2.7.4