WIP: DO NOT REVIEW Reduce Activity tag storage allocations#130610
Draft
artl93 wants to merge 2 commits into
Draft
WIP: DO NOT REVIEW Reduce Activity tag storage allocations#130610artl93 wants to merge 2 commits into
artl93 wants to merge 2 commits into
Conversation
Co-locate the first Activity tag with its list container, avoiding one allocation for every tagged Activity while preserving concurrent tag addition semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffc63197-9684-4dd6-a055-b605b57cbfd2
Contributor
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics-tracing |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR experiments with reducing allocations in System.Diagnostics.Activity tag storage by co-locating the first tag node with the tag-list container and updating tag formatting logic.
Changes:
- Refactors
Activity.TagsLinkedListto inherit fromDiagNode<KeyValuePair<string, object?>>, eliminating a separate first-node allocation. - Updates
TagsLinkedList.ToString()to useValueStringBuilderinstead of a cachedStringBuilder. - Adds a new test asserting concurrent
AddTagcalls preserve all tag values.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs | Adds a new concurrent-add tags test. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagLinkedList.cs | Makes DiagNode<T> non-sealed to enable deriving the tag list from it. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs | Implements the co-located-first-node tag list and updates tag-list string formatting. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
Comment on lines
1738
to
1749
| 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; | ||
| } |
Comment on lines
1843
to
1850
| 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; | ||
| } | ||
|
|
Comment on lines
1714
to
1718
| IEnumerator<KeyValuePair<string, object?>> e = list.GetEnumerator(); | ||
| if (!e.MoveNext()) | ||
| { | ||
| return; | ||
| } |
Avoid retaining removed or ignored tag keys and values in the co-located tag node, and cover formatting and empty-list invariants. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffc63197-9684-4dd6-a055-b605b57cbfd2
Comment on lines
+259
to
+263
| tags.GetType().GetMethod("Remove").Invoke(tags, new object[] { "key" }); | ||
|
|
||
| Assert.Null(tags.GetType().GetProperty("First").GetValue(tags)); | ||
| Assert.Equal(default, (KeyValuePair<string, object?>)tags.GetType().GetField("Value").GetValue(tags)); | ||
| } |
Comment on lines
+278
to
+280
| MethodInfo add = tags.GetType().GetMethod("Add", new Type[] { typeof(KeyValuePair<string, object>) }); | ||
| add.Invoke(tags, new object[] { new KeyValuePair<string, object?>("object", 42) }); | ||
| add.Invoke(tags, new object[] { new KeyValuePair<string, object?>("null", null) }); |
Member
Author
|
@EgorBot -linux_amd -osx_arm64 using System.Diagnostics;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
[MemoryDiagnoser]
public class ActivityTagBenchmarks
{
private ActivitySource _source = null!;
private ActivityListener _listener = null!;
private ActivityContext _parent;
private KeyValuePair<string, object?>[] _stringTags = null!;
private KeyValuePair<string, object?>[] _objectTags = null!;
[Params(0, 1, 5, 20)]
public int TagCount { get; set; }
[Params(false, true)]
public bool ObjectValues { get; set; }
[GlobalSetup]
public void Setup()
{
_source = new ActivitySource("EgorBot.Activity.Tags");
_parent = ActivityContext.Parse(
"00-0123456789abcdef0123456789abcdef-0123456789abcdef-01",
"vendor=value");
_stringTags = Enumerable.Range(0, 20)
.Select(static i => new KeyValuePair<string, object?>($"tag.{i}", $"value.{i}"))
.ToArray();
_objectTags = Enumerable.Range(0, 20)
.Select(static i => new KeyValuePair<string, object?>($"tag.{i}", new TagValue(i)))
.ToArray();
_listener = new ActivityListener
{
ShouldListenTo = static source => source.Name == "EgorBot.Activity.Tags",
Sample = static (ref ActivityCreationOptions<ActivityContext> _) =>
ActivitySamplingResult.AllDataAndRecorded,
};
ActivitySource.AddActivityListener(_listener);
}
[GlobalCleanup]
public void Cleanup()
{
_listener.Dispose();
_source.Dispose();
}
[Benchmark]
public Activity? StartStopWithTags()
{
Activity? activity = _source.StartActivity("request", ActivityKind.Server, _parent);
KeyValuePair<string, object?>[] tags = ObjectValues ? _objectTags : _stringTags;
for (int i = 0; i < TagCount; i++)
{
KeyValuePair<string, object?> tag = tags[i];
activity!.SetTag(tag.Key, tag.Value);
}
activity?.Stop();
return activity;
}
private sealed class TagValue(int value)
{
public override string ToString() => $"object-{value}";
}
}
[MemoryDiagnoser]
public class InactiveActivityBenchmarks
{
private ActivitySource _source = null!;
private ActivityListener _listener = null!;
[Params(false, true)]
public bool ListenerPresent { get; set; }
[GlobalSetup]
public void Setup()
{
_source = new ActivitySource(
ListenerPresent
? "EgorBot.Activity.Unsampled"
: "EgorBot.Activity.Disabled");
_listener = new ActivityListener
{
ShouldListenTo = static source => source.Name == "EgorBot.Activity.Unsampled",
Sample = static (ref ActivityCreationOptions<ActivityContext> _) =>
ActivitySamplingResult.None,
};
ActivitySource.AddActivityListener(_listener);
}
[GlobalCleanup]
public void Cleanup()
{
_listener.Dispose();
_source.Dispose();
}
[Benchmark]
public Activity? StartActivity() => _source.StartActivity("request");
}Note This benchmark request was generated with GitHub Copilot. |
Member
Author
Local publication benchmark updateThese runs use immutable binaries built from:
BenchmarkDotNet 0.16.0-preview.1, .NET 11, Apple M5 Pro/macOS arm64, 3 launches / 8 warmups / 12 iterations:
String and object-valued tags have identical allocation results. No-listener and listener-present-but-unsampled paths remain at zero allocation. The deterministic result is one fewer 32-byte object per tagged Activity. Timing is noisy: the 5-tag paired result is unchanged, so the earlier preliminary ~4.5% microbenchmark CPU claim is superseded and is not used in the updated description. Exact-final interleaved Kestrel support (five sampled runs/revision, 200,000 requests/run, concurrency 16):
The custom Kestrel timing is supporting evidence only. Cross-platform EgorBot results are pending. This PR remains work in progress. Note This benchmark summary was generated with GitHub Copilot. |
Member
Author
WIP statusThe local evidence pass is complete and the description now contains the exact-revision BDN/Kestrel tables, design invariants, rejected alternatives, commands, tests, and provenance hashes. Corrections made during this pass:
Current CI failures are outside this PR's files and both occur in
Build Analysis is still running, and cross-platform EgorBot results are pending. This PR remains a draft / work in progress. Do not review it yet. It will not be marked ready for review. Note This work-in-progress status update was generated with GitHub Copilot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am running an experiment - full analysis to follow. Contact @artl93
Important
This remains a draft / work in progress. Please do not review it yet.
Summary
Activitycurrently allocates two objects when its first tag is added:TagsLinkedListcontainer, andDiagNode<KeyValuePair<string, object?>>for the first value.This experiment makes
TagsLinkedListitself the first node. Later tags still use the sameDiagNodechain, so tag order, duplicate handling, lookup, mutation, and public APIs are unchanged. The result is exactly one fewer object for every tagged Activity.This targets OpenTelemetry-style sampled spans, where server/client Activities commonly carry semantic-convention tags. It does not affect histogram aggregation.
Canonical local result
Exact revisions:
7aa830a03599a8255c2c4abf2947afc5b346cc6f2a3ea726497b7d04f887a3fdb8afa4c3359bfa1dSame-process paired BenchmarkDotNet 0.16.0-preview.1, exact W3C parent, .NET 11, Apple M5 Pro/macOS arm64, 3 launches / 8 warmups / 12 measured iterations:
Separate full runs cover pre-created string and object values and produce identical allocation counts. The deterministic claim is -32 B for every tagged Activity.
The paired timing distributions are noisy: 5 tags are statistically unchanged locally, while 1 and 20 tags trend faster. I am not generalizing a microbenchmark CPU claim from these results. Earlier preliminary 5-tag timing suggested ~4.5%; the higher-confidence paired run supersedes that claim.
Disabled and listener-present-but-unsampled paths allocate zero in both revisions:
NoneThose nanosecond timings are noisy; the relevant result is no allocation or structural change on either inactive path.
Exact-final Kestrel support
A minimal in-process Kestrel client/server workload parses a remote W3C parent, starts a sampled server Activity with an OTel-like listener, and adds five tags. Five base/final runs were interleaved; each run used 200,000 requests at concurrency 16.
Timing is supporting evidence only: this is a custom in-process workload on one machine, not an official ASP.NET benchmark. The sampled allocation reduction is consistent with the deterministic 32-byte Activity saving plus request-level measurement noise. The disabled deltas are noise-sized.
This rerun uses the exact final commit, correcting the preliminary Kestrel provenance caveat noted in the original draft description.
Why this representation
TagsLinkedListalready owns list state and lifetime; co-locating the first node removes the redundant object without adding arrays, dictionaries, pools, or thresholds.SetTag/removal retain first-match behavior._last is nullmeans empty,Firstreturns null, and the co-locatedValueis cleared so removed or ignored null-set keys/values are not retained.Interlocked.CompareExchange; losing writers append under the existing list lock. A tracked 1,000-writer stress test verifies all tags are preserved.StringBuilderfield was removed.ValueStringBuilderkeeps string/object/null formatting while avoiding a retained builder field on every tagged Activity.Alternatives rejected
Activity.Currentduring sampling did not produce a qualifying exact-parent or E2E benefit and was reverted.StringBuilderwould preserve a field on every tagged Activity even though formatting is exceptional.Semantic and test coverage
Activity.Current/ async flowCommands:
./dotnet.sh build src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj --no-restore ./dotnet.sh build /t:test src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj --no-restore /p:XUnitOptions="-class System.Diagnostics.Tests.ActivityTests" ./dotnet.sh build /t:test src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj --no-restoreResults:
ActivityTests: 102 passedEvidence provenance
33b916c5d29c0258e6c9aa894e7be9e00dc15148893c2c7dba21c80b0a1cf8c65f5a72d2a01a89fabe45f8fc70e53f70e2eb4f874cfd57b135692a55651549f8629f2ed07e5c54cca1d2e47fe1135ea54ec1864f9339df62c5e969c33fc1fa41f28c46e09737c1b602259808b7ac854ca46b1e6a360d71f96ae47211749ebb18Raw local evidence includes Markdown/CSV/full JSON BDN reports, complete logs, immutable binaries/plugins, interleaved Kestrel output, environment and command manifest, source diff, test XML, and exact-final CPU/allocation traces.
Cross-platform EgorBot benchmarking has been requested in a PR comment and is still pending. This PR will remain draft regardless of that result.
Note
This work-in-progress analysis was generated with GitHub Copilot.