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

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 (file)
--- 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
     /// </summary>
     public partial class Activity
     {
+        private static readonly IEnumerable<KeyValuePair<string, string>> s_emptyBaggageTags = new KeyValuePair<string, string>[0]; // Array.Empty<T>() doesn't exist in all configurations
+
         /// <summary>
         /// 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<KeyValuePair<string, string>> 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<KeyValuePair<string, string>> 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
         /// </summary>
         public string GetBaggageItem(string key)
         {
-            foreach (var keyValue in Baggage)
+            foreach (KeyValuePair<string, string> 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 (file)
--- 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<string, string>(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<string, string>(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<string, string>("hello", "world"), anotherActivity.Baggage.First());
+                    Assert.Equal(new KeyValuePair<string, string>(Key + 2, Value + 2), anotherActivity.Baggage.Skip(1).First());
+                    Assert.Equal(new KeyValuePair<string, string>(Key + 1, Value + 1), anotherActivity.Baggage.Skip(2).First());
+                    Assert.Equal(new KeyValuePair<string, string>(Key + 0, Value + 0), anotherActivity.Baggage.Skip(3).First());
+                }
+                finally
+                {
+                    anotherActivity.Stop();
+                }
+            }
+            finally
+            {
+                activity.Stop();
+            }
         }
 
         /// <summary>
@@ -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<KeyValuePair<string, string>> 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);
+            }
         }
 
         /// <summary>