[Debugger] Improve debugger collection serialization#8753
Merged
Conversation
dudikeleti
added
the
type:performance
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Live Debugger snapshot serialization for a broader set of IEnumerable collection shapes (especially generic collections that don’t implement non-generic ICollection), while making dictionary-like types serialize consistently as key/value entries.
Changes:
- Added descriptor-based logic (with small bounded caches) to consistently read
Countand to serialize dictionary entries with stable key/value type selection. - Reclassified
SortedList/SortedList<TKey, TValue>as dictionary-like for snapshot serialization, and removedConditionalWeakTable<TKey, TValue>from supported collection handling. - Expanded test coverage to validate supported collection/dictionary shapes and specific dictionary-entry typing behavior (including null value fallback).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/test/Datadog.Trace.Tests/Debugger/SupportedTypesServiceTests.cs | Adds focused unit tests for Redaction supported collection vs dictionary classification. |
| tracer/test/Datadog.Trace.Tests/Debugger/DebuggerSnapshotCreatorTests.cs | Adds snapshot-structure assertions for supported collections/dictionaries, SortedList as dictionary entries, and ConditionalWeakTable non-support. |
| tracer/src/Datadog.Trace/Debugger/Snapshots/Redaction.cs | Refactors supported type checks to be type-based without LINQ; reclassifies SortedList as dictionary; removes ConditionalWeakTable from supported collections. |
| tracer/src/Datadog.Trace/Debugger/Snapshots/DebuggerSnapshotSerializer.cs | Switches enumerable serialization to use computed SupportedEnumerableInfo; improves dictionary entry serialization via cached descriptors and runtime-type selection. |
| tracer/src/Datadog.Trace/Debugger/Snapshots/DebuggerSnapshotSerializer.CollectionDescriptors.cs | Introduces cached descriptor helpers for supported enumerable count retrieval and dictionary entry accessors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dudikeleti
force-pushed
the
dudik/fix-debugger-collection-serialization
branch
from
June 4, 2026 17:18
2b022cc to
6bd7f9c
Compare
BenchmarksBenchmark execution time: 2026-06-04 18:10:04 Comparing candidate commit 6bd7f9c in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 61 known flaky benchmarks, 65 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8753) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (76ms) : 72, 80
master - mean (74ms) : 70, 77
section Bailout
This PR (8753) - mean (78ms) : 75, 81
master - mean (77ms) : 75, 80
section CallTarget+Inlining+NGEN
This PR (8753) - mean (1,111ms) : 1037, 1185
master - mean (1,110ms) : 1055, 1165
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (116ms) : 110, 122
master - mean (114ms) : 109, 118
section Bailout
This PR (8753) - mean (115ms) : 112, 118
master - mean (115ms) : 112, 118
section CallTarget+Inlining+NGEN
This PR (8753) - mean (797ms) : 762, 831
master - mean (796ms) : 768, 824
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (102ms) : 99, 105
master - mean (104ms) : 97, 111
section Bailout
This PR (8753) - mean (102ms) : 99, 105
master - mean (102ms) : 98, 106
section CallTarget+Inlining+NGEN
This PR (8753) - mean (959ms) : 917, 1002
master - mean (945ms) : 907, 984
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (103ms) : 97, 109
master - mean (100ms) : 96, 105
section Bailout
This PR (8753) - mean (101ms) : 98, 104
master - mean (104ms) : 97, 110
section CallTarget+Inlining+NGEN
This PR (8753) - mean (831ms) : 793, 869
master - mean (822ms) : 787, 857
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (200ms) : 195, 205
master - mean (197ms) : 190, 204
section Bailout
This PR (8753) - mean (205ms) : 201, 209
master - mean (201ms) : 196, 206
section CallTarget+Inlining+NGEN
This PR (8753) - mean (1,210ms) : 1170, 1251
master - mean (1,202ms) : 1163, 1240
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (289ms) : 280, 297
master - mean (287ms) : 279, 295
section Bailout
This PR (8753) - mean (290ms) : 284, 297
master - mean (287ms) : 281, 293
section CallTarget+Inlining+NGEN
This PR (8753) - mean (974ms) : 954, 993
master - mean (965ms) : 943, 987
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (282ms) : 275, 288
master - mean (279ms) : 274, 285
section Bailout
This PR (8753) - mean (281ms) : 276, 287
master - mean (279ms) : 275, 284
section CallTarget+Inlining+NGEN
This PR (8753) - mean (1,171ms) : 1132, 1210
master - mean (1,160ms) : 1117, 1204
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8753) - mean (279ms) : 272, 285
master - mean (278ms) : 270, 285
section Bailout
This PR (8753) - mean (278ms) : 273, 283
master - mean (278ms) : 274, 283
section CallTarget+Inlining+NGEN
This PR (8753) - mean (1,042ms) : 1003, 1080
master - mean (1,038ms) : 987, 1090
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dudikeleti
enabled auto-merge (squash)
June 4, 2026 20:44
jpbempel
approved these changes
Jun 5, 2026
Comment on lines
+32
to
+63
| private static DictionaryEntryDescriptor GetDictionaryEntryDescriptor(Type runtimeType) | ||
| { | ||
| var descriptors = _dictionaryEntryDescriptors; | ||
| for (var i = 0; i < descriptors.Length; i++) | ||
| { | ||
| if (descriptors[i].RuntimeType == runtimeType) | ||
| { | ||
| return descriptors[i]; | ||
| } | ||
| } | ||
|
|
||
| lock (DictionaryEntryDescriptorCacheLock) | ||
| { | ||
| descriptors = _dictionaryEntryDescriptors; | ||
| for (var i = 0; i < descriptors.Length; i++) | ||
| { | ||
| if (descriptors[i].RuntimeType == runtimeType) | ||
| { | ||
| return descriptors[i]; | ||
| } | ||
| } | ||
|
|
||
| var descriptor = CreateDictionaryEntryDescriptor(runtimeType); | ||
| if (descriptors.Length < MaxDictionaryEntryDescriptorCacheSize) | ||
| { | ||
| var newDescriptors = new DictionaryEntryDescriptor[descriptors.Length + 1]; | ||
| Array.Copy(descriptors, newDescriptors, descriptors.Length); | ||
| newDescriptors[descriptors.Length] = descriptor; | ||
| _dictionaryEntryDescriptors = newDescriptors; | ||
| } | ||
|
|
||
| return descriptor; |
Member
There was a problem hiding this comment.
In Java we have a CopyOnWriteArrayList with this implementation.
Worth exploring or at least adding a class for those 2 usages. this is exactly this usage.
Contributor
Author
There was a problem hiding this comment.
We have Concurrent* collections, Frozen* types, and ImmutableArray, but none of them is exactly what we want.
For this tiny cache, a raw array + lock is a good solution.
Adding a CopyOnWriteArrayList implementation sounds like a good idea. I'll do that in a follow-up PR.
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.
Summary of changes
IEnumerablecollection types that do not implement non-genericICollection.Count, dictionary keys, and dictionary values consistently.SortedList/SortedList<TKey, TValue>as dictionaryentriesinstead of collectionelements.ConditionalWeakTable<TKey, TValue>as a supported collection for debugger snapshots.Reason for change
HashSet<T>,ConcurrentQueue<T>,ConcurrentBag<T>,BlockingCollection<T>, and sorted collections.Implementation details
DebuggerSnapshotSerializer.CollectionDescriptors.cswith small bounded caches for supported enumerable descriptors and dictionary entry descriptors.Countaccessors for supported enumerable types that expose anint Countproperty.Redactionto avoid LINQ and to share type-based matching logic.Test coverage
Datadog.Trace.Tests.Debugger.DebuggerSnapshotCreatorTestsDatadog.Trace.Tests.Debugger.SupportedTypesServiceTests