Skip to content

Reduce Activity tag storage allocations #130610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
artl93 wants to merge 7 commits into from
Closed

Reduce Activity tag storage allocations #130610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4bd669f
Reduce Activity tag storage allocations
Jul 13, 2026
2a3ea72
Clear empty Activity tag storage
Jul 13, 2026
ca8c395
Address Activity tag list review feedback
Jul 19, 2026
09b7f6b
Merge upstream main for Activity readiness
Jul 19, 2026
e8b8847
Make Activity tag stress test portable
Jul 19, 2026
f4564b1
Publish empty Activity tag state before clearing
Jul 20, 2026
6e0474c
Use invariant culture in Activity tag stress test
Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 56 additions & 41 deletions src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1681,18 +1681,21 @@ public void Remove(string key)
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

internal sealed class TagsLinkedList : IEnumerable<KeyValuePair<string, object?>>
internal sealed class TagsLinkedList : DiagNode<KeyValuePair<string, object?>>, IEnumerable<KeyValuePair<string, object?>>
{
private DiagNode<KeyValuePair<string, object?>>? _first;
private DiagNode<KeyValuePair<string, object?>>? _last;

private StringBuilder? _stringBuilder;

public TagsLinkedList(KeyValuePair<string, object?> firstValue, bool set = false) => _last = _first = ((set && firstValue.Value == null) ? null : new DiagNode<KeyValuePair<string, object?>>(firstValue));
public TagsLinkedList(KeyValuePair<string, object?> firstValue, bool set = false) : base(set && firstValue.Value is null ? default : firstValue)
{
if (!set || firstValue.Value is not null)
{
_last = this;
}
}

public TagsLinkedList(IEnumerator<KeyValuePair<string, object?>> e)
public TagsLinkedList(IEnumerator<KeyValuePair<string, object?>> e) : base(e.Current)
{
_last = _first = new DiagNode<KeyValuePair<string, object?>>(e.Current);
_last = this;

while (e.MoveNext())
{
Expand All @@ -1701,22 +1704,23 @@ public TagsLinkedList(IEnumerator<KeyValuePair<string, object?>> e)
}
}

public DiagNode<KeyValuePair<string, object?>>? First => _first;
public DiagNode<KeyValuePair<string, object?>>? First => _last is null ? null : this;

public TagsLinkedList(IEnumerable<KeyValuePair<string, object?>> list) => Add(list);
public TagsLinkedList(IEnumerable<KeyValuePair<string, object?>> list) : base(default!) => Add(list);
Comment thread
artl93 marked this conversation as resolved.

// Add doesn't take the lock because it is called from the Activity creation before sharing the activity object to the caller.
public void Add(IEnumerable<KeyValuePair<string, object?>> list)
{
IEnumerator<KeyValuePair<string, object?>> e = list.GetEnumerator();
using IEnumerator<KeyValuePair<string, object?>> e = list.GetEnumerator();
if (!e.MoveNext())
{
return;
}

if (_first == null)
if (_last is null)
{
_last = _first = new DiagNode<KeyValuePair<string, object?>>(e.Current);
Value = e.Current;
_last = this;
}
else
{
Expand All @@ -1733,18 +1737,18 @@ public void Add(IEnumerable<KeyValuePair<string, object?>> list)

public void Add(KeyValuePair<string, object?> value)
{
DiagNode<KeyValuePair<string, object?>> newNode = new DiagNode<KeyValuePair<string, object?>>(value);

lock (this)
{
if (_first == null)
if (_last is null)
{
_first = _last = newNode;
Value = value;
_last = this;
return;
}

Debug.Assert(_last != null);

DiagNode<KeyValuePair<string, object?>> newNode = new DiagNode<KeyValuePair<string, object?>>(value);
_last!.Next = newNode;
_last = newNode;
}
Expand All @@ -1753,7 +1757,7 @@ public void Add(KeyValuePair<string, object?> value)
public object? Get(string key)
{
// We don't take the lock here so it is possible the Add/Remove operations mutate the list during the Get operation.
DiagNode<KeyValuePair<string, object?>>? current = _first;
DiagNode<KeyValuePair<string, object?>>? current = First;
while (current != null)
{
if (current.Value.Key == key)
Expand All @@ -1771,21 +1775,32 @@ public void Remove(string key)
{
lock (this)
{
if (_first == null)
if (_last is null)
{
return;
}
if (_first.Value.Key == key)

if (Value.Key == key)
{
_first = _first.Next;
if (_first is null)
DiagNode<KeyValuePair<string, object?>>? next = Next;
if (next is null)
{
_last = null;
Value = default;
}
else
{
Value = next.Value;
Next = next.Next;
if (object.ReferenceEquals(_last, next))
{
_last = this;
}
}
return;
}

DiagNode<KeyValuePair<string, object?>> previous = _first;
DiagNode<KeyValuePair<string, object?>> previous = this;

while (previous.Next != null)
{
Expand Down Expand Up @@ -1813,7 +1828,7 @@ public void Set(KeyValuePair<string, object?> value)

lock (this)
{
DiagNode<KeyValuePair<string, object?>>? current = _first;
DiagNode<KeyValuePair<string, object?>>? current = First;
while (current != null)
{
if (current.Value.Key == value.Key)
Expand All @@ -1825,27 +1840,28 @@ public void Set(KeyValuePair<string, object?> value)
current = current.Next;
}

DiagNode<KeyValuePair<string, object?>> newNode = new DiagNode<KeyValuePair<string, object?>>(value);
if (_first == null)
if (_last is null)
{
_first = _last = newNode;
Value = value;
_last = this;
return;
}

Debug.Assert(_last != null);

DiagNode<KeyValuePair<string, object?>> newNode = new DiagNode<KeyValuePair<string, object?>>(value);
_last!.Next = newNode;
_last = newNode;
}
}

public DiagEnumerator<KeyValuePair<string, object?>> GetEnumerator() => new DiagEnumerator<KeyValuePair<string, object?>>(_first);
public DiagEnumerator<KeyValuePair<string, object?>> GetEnumerator() => new DiagEnumerator<KeyValuePair<string, object?>>(First);
IEnumerator<KeyValuePair<string, object?>> IEnumerable<KeyValuePair<string, object?>>.GetEnumerator() => GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

public IEnumerable<KeyValuePair<string, string?>> EnumerateStringValues()
{
DiagNode<KeyValuePair<string, object?>>? current = _first;
DiagNode<KeyValuePair<string, object?>>? current = First;

while (current != null)
{
Expand All @@ -1862,30 +1878,29 @@ public override string ToString()
{
lock (this)
{
if (_first == null)
DiagNode<KeyValuePair<string, object?>>? first = First;
if (first is null)
{
return string.Empty;
}

_stringBuilder ??= new StringBuilder();
_stringBuilder.Append(_first.Value.Key);
_stringBuilder.Append(':');
_stringBuilder.Append(_first.Value.Value);
var stringBuilder = new ValueStringBuilder(stackalloc char[256]);
stringBuilder.Append(first.Value.Key);
stringBuilder.Append(':');
stringBuilder.Append(first.Value.Value?.ToString());

DiagNode<KeyValuePair<string, object?>>? current = _first.Next;
DiagNode<KeyValuePair<string, object?>>? current = first.Next;
while (current != null)
{
_stringBuilder.Append(", ");
_stringBuilder.Append(current.Value.Key);
_stringBuilder.Append(':');
_stringBuilder.Append(current.Value.Value);
stringBuilder.Append(", ");
stringBuilder.Append(current.Value.Key);
stringBuilder.Append(':');
stringBuilder.Append(current.Value.Value?.ToString());

current = current.Next;
}

string result = _stringBuilder.ToString();
_stringBuilder.Clear();
return result;
return stringBuilder.ToString();
Comment thread
artl93 marked this conversation as resolved.
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagLinkedList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

namespace System.Diagnostics
{
internal sealed partial class DiagNode<T>
internal partial class DiagNode<T>
Comment thread
artl93 marked this conversation as resolved.
{
public DiagNode(T value) => Value = value;
public T Value;
Expand Down
64 changes: 64 additions & 0 deletions src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
Expand Down Expand Up @@ -233,6 +234,69 @@ public void Tags()
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))]
public void ConcurrentTagAddsPreserveAllValues()
{
const int Count = 1_000;
Activity activity = new Activity("activity");

Parallel.For(0, Count, i => activity.AddTag(i.ToString(CultureInfo.InvariantCulture), i));
Comment thread
artl93 marked this conversation as resolved.

KeyValuePair<string, object?>[] tags = activity.TagObjects.ToArray();
Assert.Equal(Count, tags.Length);
Assert.Equal(Count, tags.Select(tag => tag.Key).Distinct().Count());
foreach (KeyValuePair<string, object?> tag in tags)
{
Assert.Equal(int.Parse(tag.Key, CultureInfo.InvariantCulture), tag.Value);
}
Comment thread
artl93 marked this conversation as resolved.
}

[Fact]
public void TagsLinkedListClearsLastRemovedValue()
{
object value = new object();
object tags = CreateTagsLinkedList(new KeyValuePair<string, object?>("key", value));

MethodInfo remove = Assert.IsAssignableFrom<MethodInfo>(tags.GetType().GetMethod("Remove"));
PropertyInfo first = Assert.IsAssignableFrom<PropertyInfo>(tags.GetType().GetProperty("First"));
FieldInfo storedValue = Assert.IsAssignableFrom<FieldInfo>(tags.GetType().GetField("Value"));
remove.Invoke(tags, new object[] { "key" });

Assert.Null(first.GetValue(tags));
Assert.Equal(default, (KeyValuePair<string, object?>)storedValue.GetValue(tags));
}

[Fact]
public void TagsLinkedListDoesNotRetainIgnoredSetValue()
{
object tags = CreateTagsLinkedList(new KeyValuePair<string, object?>("key", null), set: true);
PropertyInfo first = Assert.IsAssignableFrom<PropertyInfo>(tags.GetType().GetProperty("First"));
FieldInfo storedValue = Assert.IsAssignableFrom<FieldInfo>(tags.GetType().GetField("Value"));

Assert.Null(first.GetValue(tags));
Assert.Equal(default, (KeyValuePair<string, object?>)storedValue.GetValue(tags));
}

[Fact]
public void TagsLinkedListToStringFormatsValues()
{
object tags = CreateTagsLinkedList(new KeyValuePair<string, object?>("string", "value"));
MethodInfo add = Assert.IsAssignableFrom<MethodInfo>(tags.GetType().GetMethod("Add", new Type[] { typeof(KeyValuePair<string, object>) }));
Comment thread
artl93 marked this conversation as resolved.
add.Invoke(tags, new object[] { new KeyValuePair<string, object?>("object", 42) });
add.Invoke(tags, new object[] { new KeyValuePair<string, object?>("null", null) });

Assert.Equal("string:value, object:42, null:", tags.ToString());
}

private static object CreateTagsLinkedList(KeyValuePair<string, object?> firstValue, bool set = false)
{
Type? tagsType = typeof(Activity).GetNestedType("TagsLinkedList", BindingFlags.NonPublic);
Assert.NotNull(tagsType);
object? tags = Activator.CreateInstance(tagsType, new object[] { firstValue, set });
Assert.NotNull(tags);
return tags;
}

/// <summary>
/// Tests activity SetParentId
/// </summary>
Expand Down
Loading