diff --git a/.claude/skills/find-allocation-opportunities/SKILL.md b/.claude/skills/find-allocation-opportunities/SKILL.md new file mode 100644 index 000000000000..c500e02d8952 --- /dev/null +++ b/.claude/skills/find-allocation-opportunities/SKILL.md @@ -0,0 +1,145 @@ +--- +name: find-allocation-opportunities +description: >- + This skill should be used when the user asks to "find allocations", "allocation review", + "heap allocation", "reduce allocations", "optimize allocations", "check for allocations", + "allocation opportunities", "avoid boxing", "avoid allocation", "zero-alloc", "hot path + review", "GC pressure", "reduce GC", "allocation-free", "avoid heap", "memory pressure", + or mentions reviewing code for allocation overhead. Also trigger when reviewing PRs that + touch hot paths (span creation, context propagation, instrumentation callbacks, + serialization) and the user wants to check for unnecessary allocations. Covers both + scanning existing code and reviewing diffs/PRs for missed optimization opportunities. +argument-hint: "[pr | ]" +--- + +# Find Allocation Opportunities + +## Why This Matters + +The dd-trace-dotnet tracer runs **in-process** with customer applications. Every heap allocation +in hot paths adds GC pressure to the customer's app. The team has developed 22+ proven patterns +for avoiding allocations — this skill codifies that knowledge for consistent application. + +## Critical Code Paths + +Not all code needs aggressive optimization. Focus effort based on path temperature: + +1. **Hot paths** (highest priority): Span creation/tagging, context propagation, sampling + decisions, instrumentation callbacks (CallTarget `OnMethodBegin`/`OnMethodEnd`), request/response + pipeline, MessagePack serialization +2. **Startup/bootstrap** (medium priority): Managed loader, tracer initialization, static + constructors, configuration loading, integration registration +3. **Cold paths** (lower priority): One-time setup, error handling, diagnostic logging, + configuration changes — still worth optimizing but not critical + +## Modes of Operation + +### PR Review Mode + +Given a PR number, diff, or set of changed files: + +1. Fetch the diff (or read the provided files) +2. Identify which changed code touches hot paths vs. cold paths +3. Read `references/anti-patterns.md` and scan the diff for matches +4. For each finding, look up the appropriate fix in `references/patterns.md` +5. Report findings prioritized by path temperature + +### Codebase Scan Mode + +Given a file, directory, or glob pattern: + +1. Read the target files (exclude `Vendors/`, `Generated/`, and `test/` directories) +2. Classify path temperature using Step 1 below +3. Read `references/anti-patterns.md` and search for matches using Grep +4. For each finding, look up the appropriate fix in `references/patterns.md` +5. Report findings prioritized by path temperature + +## Workflow + +### Step 1: Determine scope and temperature + +Classify the target code: +- Files under `ClrProfiler/`, `Agent/MessagePack/`, `Propagators/`, `Tagging/`, `Sampling/`, + `Processors/` = hot path +- Files under `Configuration/`, `ClrProfiler/Managed.Loader/` = startup path +- Everything else = assess based on call frequency + +### Step 2: Load reference material + +Read the appropriate reference files based on what the scan reveals: +- **`references/anti-patterns.md`** — The anti-patterns to search for, with grep patterns +- **`references/patterns.md`** — The proven optimization patterns with concrete examples from + this codebase + +### Step 3: Search for anti-patterns + +Use Grep to search for the anti-patterns documented in `references/anti-patterns.md`. Each +anti-pattern entry includes suggested search patterns. See the "Search Guidelines" section +at the top of that file for exclusion rules (`Vendors/`, `Generated/`, `test/`). + +### Step 4: Assess and report + +For each finding, determine: +- **Does this actually cause a heap allocation?** Only report findings that produce heap + allocations (object/array/string/delegate/boxing). Do not report redundant calls, style + issues, or other inefficiencies that don't allocate — those are out of scope for this skill. +- Is this on a hot path or eager initialization path? (Check callers if unclear) + Hot-path allocations are highest priority. Eager initialization allocations matter too — + they affect cold-start times. Prefer lazy initialization when possible. True one-shot + cold-path allocations (error handling, rare config changes) are lowest priority. +- Is there already an optimization in place? (e.g., the allocation might be behind an `IsEnabled` guard) +- What's the concrete fix using an existing pattern from this codebase? + +### Step 5: Format output + +Report each finding as: + +``` +## [Priority: High/Medium/Low] — [Anti-pattern name] + +**Location**: `file_path:line_number` +**Path temperature**: Hot / Startup / Cold +**Issue**: [Brief description of the allocation] +**Fix**: [Concrete code change using a pattern from references/patterns.md] + +Before: +```csharp +// allocating code +``` + +After: +```csharp +// optimized code +``` +``` + +Group findings by priority. Include a summary count at the top. + +### Step 6: Verify fixes compile + +When suggesting concrete code changes, verify they compile for all target frameworks: +``` +dotnet build tracer/src/Datadog.Trace/ -c Release -f net6.0 +``` +Drop the `-f` flag to test all TFMs if the change involves `#if` guards. + +## Important Caveats + +- Readability matters — not every allocation needs eliminating. Focus on hot paths and + frequently-called code. +- **Check `#if` preprocessor context**: Many patterns (`Span`, `stackalloc`, + `ValueStringBuilder`, `ValueTask`, `SpanCharSplitter`) require `NETCOREAPP` or + `NETCOREAPP3_1_OR_GREATER`. Before suggesting these patterns, check whether the code is + inside an `#if NETFRAMEWORK` block. When the code must work on both runtimes, provide + both the optimized path and the fallback wrapped in `#if` / `#else`. +- `stackalloc` buffers should have a reasonable size threshold (typically 256-512 bytes). + Fall back to `ArrayPool` for larger buffers. +- Verify that "optimized" code actually compiles for all target frameworks before suggesting it. + +## Reference Files + +- **`references/patterns.md`** — Complete catalog of 22+ allocation-avoidance patterns used in + this codebase, organized by category with concrete file:line examples +- **`references/anti-patterns.md`** — Anti-patterns to detect, with grep search patterns and + suggested fixes. Covers both tracer-specific patterns (from AGENTS.md Performance Guidelines) + and general .NET allocation pitfalls diff --git a/.claude/skills/find-allocation-opportunities/references/anti-patterns.md b/.claude/skills/find-allocation-opportunities/references/anti-patterns.md new file mode 100644 index 000000000000..d9a5d3c2a56b --- /dev/null +++ b/.claude/skills/find-allocation-opportunities/references/anti-patterns.md @@ -0,0 +1,393 @@ +# Anti-Patterns: Allocations to Flag + +This file lists allocation-producing patterns to search for, organized by severity and +detectability. Each entry includes grep-friendly search patterns and the recommended fix. + +## Search Guidelines + +When using Grep to search for these anti-patterns: +- **Always exclude** `Vendors/`, `Generated/`, and `test/` directories from results — these + are vendored code, auto-generated code, and test code respectively +- **Check `#if` preprocessor context** around findings. Many optimization patterns (e.g., + `Span`, `stackalloc`, `ValueTask`) require `NETCOREAPP` or `NETCOREAPP3_1_OR_GREATER`. + Do not suggest TFM-specific fixes inside `#if NETFRAMEWORK` blocks +- **Verify path temperature** before reporting — an anti-pattern in a cold path is low priority + +--- + +## Tracer-Specific Anti-Patterns + +These come from the dd-trace-dotnet Performance Guidelines and are highest priority. + +### AP-1: String Interpolation in Log Calls + +**What allocates**: The interpolated string is constructed even if the log level is disabled. +On .NET Framework, also allocates a `string` + boxes value types. + +**Search patterns**: +``` +Log\.\w+\(\$" +Log\.\w+\(.*\$" +``` + +**Fix**: Use format strings with positional placeholders: +```csharp +// Bad: +Log.Debug($"Processing span {spanId} with tag {tagName}"); + +// Good: +Log.Debug("Processing span {SpanId} with tag {TagName}", spanId, tagName); +``` + +**Reference**: Pattern 6.1 (Generic Log Overloads) + +### AP-2: `ToString()` on Numeric Types in Log Calls + +**What allocates**: Unnecessarily converts to string, allocating a `string` object. The +generic log overloads handle formatting without allocation. + +**Search patterns**: +``` +Log\.\w+.*\.ToString\(\) +``` + +**Fix**: Pass the value directly: +```csharp +// Bad: +Log.Debug("Error (attempt {Attempt})", (attempt + 1).ToString()); + +// Good: +Log.Debug("Error (attempt {Attempt})", attempt + 1); +``` + +### AP-3: `params` Array Without Typed Overloads + +**What allocates**: A new `object[]` on every call, plus boxing of any value-type arguments. + +**Search patterns**: Look for public/internal methods with `params object[]` that are called +from hot paths without corresponding typed overloads. + +**Fix**: Provide typed overloads for common arities (0-5 args): +```csharp +// Instead of only: +void Log(string template, params object[] args); + +// Also provide: +void Log(string template, T arg); +void Log(string template, T0 arg0, T1 arg1); +``` + +**Reference**: Pattern 6.1 (Generic Log Overloads), `Logging/Internal/DatadogSerilogLogger.cs` + +### AP-4: Boxing Through Interface Dispatch + +**What allocates**: A boxed copy of the struct on every interface method call. + +**Search patterns**: Look for methods that accept an interface parameter and are called with +struct arguments without a generic constraint: +``` +void Process(IMyInterface item) // boxing occurs when called with a struct +``` + +**Fix**: Use generic constraints: +```csharp +// Bad: +void Inject(ICarrierSetter setter) // boxes struct callers + +// Good: +void Inject(TSetter setter) where TSetter : struct, ICarrierSetter +``` + +**Reference**: Pattern 3.4 (Generic Struct Constraints) + +--- + +## General .NET Allocation Pitfalls + +### AP-5: LINQ in Hot Paths + +**What allocates**: Iterator objects, delegate objects for lambdas, sometimes intermediate +collections. Each LINQ method in a chain allocates at least one iterator. + +**Search patterns**: +``` +\.Where\( +\.Select\( +\.Any\( +\.First\( +\.OrderBy\( +\.ToList\(\) +\.ToArray\(\) +\.ToDictionary\( +``` + +**Fix**: Replace with `foreach` loops or array indexing: +```csharp +// Bad (3 allocations: Where iterator + Select iterator + ToList): +var names = items.Where(x => x.IsActive).Select(x => x.Name).ToList(); + +// Good (1 allocation: the list itself): +var names = new List(items.Count); +foreach (var item in items) +{ + if (item.IsActive) + names.Add(item.Name); +} +``` + +**Note**: LINQ is fine in cold paths (configuration, startup, error handling). Only flag it +in hot paths. + +### AP-6: String Concatenation in Loops + +**What allocates**: A new `string` on each `+` or `+=` operation. + +**Search patterns**: Look for `+=` on string variables inside loops, or repeated `string.Concat` +/ `string.Format` calls. + +**Fix**: Use `StringBuilderCache` or `ValueStringBuilder`: +```csharp +// Bad: +string result = ""; +foreach (var item in items) + result += item.Name + ","; + +// Good: +var sb = StringBuilderCache.Acquire(); +foreach (var item in items) + sb.Append(item.Name).Append(','); +return StringBuilderCache.GetStringAndRelease(sb); +``` + +**Reference**: Pattern 2.3 (`StringBuilderCache`), Pattern 1.2 (`ValueStringBuilder`) + +### AP-7: Closures Capturing Local Variables + +**What allocates**: The compiler generates a display class (heap object) to hold captured +variables. A new delegate is also allocated. + +**Search patterns**: Lambdas inside hot-path methods that reference local variables or `this`: +```csharp +// Look for lambdas that use variables from the enclosing scope: +var threshold = GetThreshold(); +items.Where(x => x.Value > threshold); // captures 'threshold' +``` + +**Fix**: +- Use `static` lambdas (C# 9+) when possible — the compiler rejects accidental captures +- Pass state through method parameters instead of closures +- Cache the delegate in a `static readonly` field if it doesn't capture +- For `Task.ContinueWith` and similar, use the `state` parameter overload + +**Reference**: Pattern 6.2 (Static / Cached Delegates) + +### AP-8: `Substring` Instead of `Span` Slicing + +**What allocates**: A new `string` for each `Substring` call. + +**Search patterns**: +``` +\.Substring\( +``` + +**Fix**: Use `AsSpan()` / `ReadOnlySpan` for intermediate parsing: +```csharp +// Bad: +var host = url.Substring(0, colonIndex); +var port = url.Substring(colonIndex + 1); + +// Good: +ReadOnlySpan host = url.AsSpan(0, colonIndex); +ReadOnlySpan port = url.AsSpan(colonIndex + 1); +``` + +**Caveat**: Only if the result doesn't need to be stored as a `string`. If you ultimately +need a `string`, the allocation is unavoidable. + +**Reference**: Pattern 4.2 (`ReadOnlySpan` / `.AsSpan()`) + +### AP-9: `Enum.ToString()` and `Enum.HasFlag()` Boxing + +**What allocates**: On .NET Framework and older runtimes, `Enum.ToString()` boxes the enum +value. `Enum.HasFlag()` boxes both operands before .NET 7. + +**Search patterns**: +``` +\.ToString\(\) // on enum-typed variables +\.HasFlag\( +``` + +**Fix**: +- For `ToString()`: Use a switch expression or lookup dictionary +- For `HasFlag()`: Use bitwise operations: `(flags & MyEnum.Value) != 0` + +**Note**: .NET 7+ JIT optimizes `HasFlag` to avoid boxing. `ToString()` is still not free. + +**Search caveat**: The `\.ToString\(\)` pattern is very broad — it matches all `ToString()` +calls, not just enum ones. Manually verify the variable is an enum type, or narrow the search +to known enum types in the codebase (e.g., `SpanKind`, `SamplingPriority`). + +### AP-10: `IEnumerable` Return Types on Hot Methods + +**What allocates**: Returning `IEnumerable` forces callers to use the interface +`GetEnumerator()`, which boxes struct enumerators. Also prevents callers from using +`Count` or indexing without `ToList()`. + +**Search patterns**: Methods on hot paths returning `IEnumerable` where a concrete type +would work. + +**Fix**: Return concrete types (`List`, `T[]`, or a custom struct collection): +```csharp +// Bad: +IEnumerable GetSpans() => _spans; + +// Good: +SpanCollection GetSpans() => _spans; // SpanCollection is a readonly struct +``` + +**Reference**: Pattern 3.3 (Struct Enumerators) + +### AP-11: Missing `JsonArrayPool` on Newtonsoft.Json Readers/Writers + +**What allocates**: Newtonsoft.Json internally allocates `char[]` buffers for tokenization. + +**Search patterns**: +``` +new JsonTextReader\( +new JsonTextWriter\( +``` +Then check if `ArrayPool = JsonArrayPool.Shared` is set. + +**Fix**: +```csharp +using var reader = new JsonTextReader(sr) { ArrayPool = JsonArrayPool.Shared }; +``` + +**Reference**: Pattern 2.4 (`JsonArrayPool`) + +### AP-12: `new StringBuilder()` Instead of `StringBuilderCache` + +**What allocates**: A new `StringBuilder` on each call. + +**Search patterns**: +``` +new StringBuilder\( +``` + +**Fix**: Use `StringBuilderCache.Acquire()` / `GetStringAndRelease()` for strings under 360 +chars. For longer strings or nested usage, `new StringBuilder()` is fine. + +**Reference**: Pattern 2.3 (`StringBuilderCache`) + +### AP-13: Throwing Exceptions Inline in Hot Paths + +**What allocates**: The `throw` statement and exception object construction can prevent the +JIT from inlining the containing method. + +**Search patterns**: `throw new` inside small methods that should be inlineable. + +**Fix**: Move to a `ThrowHelper` method: +```csharp +// Bad: +if (value is null) throw new ArgumentNullException(nameof(value)); + +// Good: +if (value is null) ThrowHelper.ThrowArgumentNullException(nameof(value)); +``` + +**Reference**: Pattern 5.2 (`ThrowHelper` with `[NoInlining]`) + +### AP-14: `Task.Run` / `Task.ContinueWith` with Capturing Lambdas + +**What allocates**: A display class for captures + a delegate + the `Task` wrapper. + +**Search patterns**: +``` +Task\.Run\( +\.ContinueWith\( +``` + +**Fix**: Use the `state` parameter overload to pass data without closures: +```csharp +// Bad: +Task.Run(() => Process(item)); + +// Good: +Task.Factory.StartNew(static state => Process((Item)state), item); +``` + +Or restructure to avoid `Task.Run` entirely in hot paths. + +**Note**: `Task.Run` and `ContinueWith` are fine in cold paths (startup, configuration). +Only flag in hot paths. + +### AP-15: `string.Split()` in Hot Paths + +**What allocates**: `string.Split()` allocates a `string[]` plus a new `string` for each +segment — multiple heap allocations per call. + +**Search patterns**: +``` +\.Split\( +``` + +**Fix**: Use `SpanCharSplitter` (NETCOREAPP) or manual `IndexOf`-based parsing: +```csharp +// Bad: +foreach (var part in input.Split(',')) + Process(part); + +// Good (NETCOREAPP — zero-alloc): +foreach (var part in new SpanCharSplitter(input, ',', int.MaxValue)) + Process(part.AsSpan()); + +// Good (.NET Framework — manual parsing): +int start = 0; +int idx; +while ((idx = input.IndexOf(',', start)) >= 0) +{ + Process(input, start, idx - start); + start = idx + 1; +} +Process(input, start, input.Length - start); +``` + +**Reference**: Pattern 4.3 (`string.Split()` Replacement with `SpanCharSplitter`) + +### AP-16: `async Task` Where `ValueTask` Would Suffice + +**What allocates**: Every `async Task` method allocates a `Task` object and an async state +machine on the heap, even when the method completes synchronously (e.g., cache hit, early +return). + +**Search patterns**: Look for `async Task` or `async Task<` methods on hot paths that have +early synchronous return paths (cache lookups, guard clauses, already-computed results). + +**Fix**: Return `ValueTask` / `ValueTask` to avoid the `Task` allocation on synchronous +completion: +```csharp +#if NETCOREAPP3_1_OR_GREATER +internal ValueTask GetValueAsync() +{ + if (_cache.TryGetValue(key, out var value)) + return new ValueTask(value); // no heap allocation + + return GetValueSlowAsync(); +} +#else +internal Task GetValueAsync() +{ + if (_cache.TryGetValue(key, out var value)) + return Task.FromResult(value); + + return GetValueSlowAsync(); +} +#endif +``` + +**Caveats**: +- `ValueTask` requires `NETCOREAPP3_1_OR_GREATER` for pooled async builders — use `#if` guards +- `ValueTask` can only be awaited once — do not cache or await multiple times +- Only beneficial when synchronous completion is common; use `Task` for always-async operations + +**Reference**: Pattern 6.3 (`ValueTask` / `ValueTask` for Hot Async Paths) diff --git a/.claude/skills/find-allocation-opportunities/references/patterns.md b/.claude/skills/find-allocation-opportunities/references/patterns.md new file mode 100644 index 000000000000..bf962145320e --- /dev/null +++ b/.claude/skills/find-allocation-opportunities/references/patterns.md @@ -0,0 +1,489 @@ +# Allocation-Avoidance Pattern Catalog + +This catalog documents proven patterns used throughout `tracer/src/Datadog.Trace/` to minimize +heap allocations. Each pattern includes what allocation it avoids, when to use it, concrete +examples from the codebase, and caveats. + +--- + +## Category 1: Stack Allocation + +### 1.1 `stackalloc` with `Span` + +**Avoids**: Heap-allocated `byte[]` or `char[]` for small temporary buffers. + +**When to use**: Small, fixed-size buffers (typically under 256-512 bytes) whose lifetime +doesn't escape the method. + +**Examples**: +- `Util/FnvHash64.cs:65` — `Span bytes = stackalloc byte[MaxStackLimit]` +- `Propagators/W3CBaggagePropagator.cs:125` — `Span stackBuffer = stackalloc byte[maxByteCount]` +- `Agent/DiscoveryService/DiscoveryService.cs:334` — `Span buffer = stackalloc byte[10]` +- `Util/HexString.cs:90` — `var bytesPtr = stackalloc byte[8]` + +**Pattern**: Use a size threshold and fall back to `ArrayPool` for large inputs. Track the +rented array separately so it can be returned: +```csharp +const int MaxStackLimit = 256; +byte[]? rented = inputSize > MaxStackLimit ? ArrayPool.Shared.Rent(inputSize) : null; +Span buffer = rented ?? stackalloc byte[MaxStackLimit]; +try +{ + // use buffer[..actualSize] +} +finally +{ + if (rented is not null) + ArrayPool.Shared.Return(rented); +} +``` + +**Caveats**: +- Requires `NETCOREAPP` — use `#if` guards for .NET Framework support +- Never stackalloc large or variable-size buffers (risk of stack overflow) +- Use a constant for the stackalloc size, not a variable (JIT optimization) + +### 1.2 `ValueStringBuilder` + +**Avoids**: `StringBuilder` heap allocation for string building operations. + +**When to use**: Building strings from multiple parts where the result is typically under ~512 +chars. Especially useful in formatting resource names, URLs, and header values. + +**Implementation**: `Util/ValueStringBuilder.cs` — a `ref struct` backed by `Span` from +`stackalloc`, falling back to `ArrayPool` if it grows. + +**Examples**: +- `DiagnosticListeners/AspNetCoreResourceNameHelper.cs:32`: + ```csharp + var sb = totalLength <= 512 + ? new ValueStringBuilder(stackalloc char[512]) + : new ValueStringBuilder(); // falls back to ArrayPool + ``` + +**Caveats**: Only available on `NETCOREAPP` (it's a `ref struct` using `Span`). + +### 1.3 `[SkipLocalsInit]` + +**Avoids**: The JIT's default zero-initialization of local variables, saving a `memset` on +`stackalloc` buffers. + +**When to use**: Methods with `stackalloc` where the buffer will be fully written before reading. + +**Implementation**: `Util/System.Runtime.CompilerServices.Attributes.cs:47` — polyfill for +older TFMs. + +**Examples**: +- `Util/FnvHash64.cs:52` — `[System.Runtime.CompilerServices.SkipLocalsInit]` +- `DataStreamsMonitoring/Hashes/HashHelper.cs:65` + +**Caveats**: Only skip init when the buffer is guaranteed to be written before read. Incorrect +use leads to reading uninitialized memory. + +--- + +## Category 2: Pooling and Caching + +### 2.1 `ArrayPool.Shared` + +**Avoids**: Repeated `new T[]` allocations for temporary arrays. + +**When to use**: Temporary arrays that are too large for `stackalloc` or when the code must +support .NET Framework. + +**Examples**: +- `Debugger/Symbols/Utf8CountingPooledTextWriter.cs:42` — `ArrayPool.Shared.Rent(initialCapacity)` +- `Debugger/Symbols/SymbolExtractor.cs:189` — `ArrayPool.Shared.Rent(scopesBufferLength)` +- `Debugger/SpanCodeOrigin/SpanCodeOrigin.cs:256` — `ArrayPool.Shared.Rent(...)` +- `OpenTelemetry/Metrics/TagSet.cs:51` — `ArrayPool>.Shared.Rent(len)` + +**Pattern**: Always return in a `finally` block: +```csharp +var rented = ArrayPool.Shared.Rent(size); +try +{ + // use rented[0..actualSize] +} +finally +{ + ArrayPool.Shared.Return(rented, clearArray: containsSensitiveData); +} +``` + +### 2.2 `FixedSizeArrayPool` + +**Avoids**: Repeated small array allocations (1-5 elements) — lighter weight than `ArrayPool` +for tiny arrays. + +**Implementation**: `Util/FixedSizeArrayPool.cs` — fast `Interlocked.Exchange` path with +`ConcurrentStack` overflow. Returns a `ref struct ArrayPoolItem` for RAII disposal. + +**Examples**: +- `Logging/Internal/DatadogSerilogLogger.cs:219-284` — used by generic log overloads to + avoid `params object[]` allocation: + ```csharp + using var array = FixedSizeArrayPool.TwoItemPool.Get(); + array.Array[0] = property0; + array.Array[1] = property1; + ``` + +**When to use**: High-frequency code that needs 1-5 element arrays (e.g., logging arguments). + +### 2.3 `StringBuilderCache` + +**Avoids**: `new StringBuilder()` allocation per string-building operation. + +**Implementation**: `Util/StringBuilderCache.cs` — `[ThreadStatic]` cached `StringBuilder` +with acquire/release pattern. Max capacity: 360 chars. + +**Examples**: Used in 57+ files across the codebase. + +**Pattern**: +```csharp +var sb = StringBuilderCache.Acquire(); +sb.Append("prefix"); +sb.Append(value); +return StringBuilderCache.GetStringAndRelease(sb); +``` + +**Caveats**: Not nestable on the same thread — if the called method also uses +`StringBuilderCache`, one of them will allocate a new `StringBuilder`. + +### 2.4 `JsonArrayPool` + +**Avoids**: Newtonsoft.Json's internal `char[]` allocations during JSON read/write. + +**Implementation**: `Util/Json/JsonArrayPool.cs` — wraps `ArrayPool.Shared` as +Newtonsoft's `IArrayPool`. + +**Examples** (24 files): +```csharp +using var jsonReader = new JsonTextReader(reader) { ArrayPool = JsonArrayPool.Shared }; +using var jsonWriter = new JsonTextWriter(writer) { ArrayPool = JsonArrayPool.Shared }; +``` + +**When to use**: Every `JsonTextReader` and `JsonTextWriter` creation. + +### 2.5 `UnmanagedMemoryPool` + +**Avoids**: GC pressure entirely by pooling native memory blocks via `Marshal.AllocCoTaskMem`. + +**Implementation**: `Util/UnmanagedMemoryPool.cs` — not thread-safe, designed for use with +`[ThreadStatic]`. + +**Examples**: +- `AppSec/WafEncoding/Encoder.cs:32` — `[ThreadStatic]` unmanaged memory pool for WAF encoding + +**When to use**: High-frequency native interop where GC pauses are unacceptable. + +### 2.6 `[ThreadStatic]` Caching + +**Avoids**: Both allocation and lock contention by keeping per-thread cached instances. + +**Examples**: +- `Util/StringBuilderCache.cs:23` — cached `StringBuilder` +- `Agent/MessagePack/MessagePackStringCache.cs:23-37` — cached serialized byte arrays +- `AppSec/WafEncoding/Encoder.cs:32` — cached `UnmanagedMemoryPool` +- `Util/ThreadSafeRandom.cs:19` — cached `Random` instance + +**Caveats**: Memory scales linearly with thread count. Best for small, frequently-reused objects. + +### 2.7 `MessagePackStringCache` + +**Avoids**: Repeated MessagePack encoding of semi-static strings (env, version, service name). + +**Implementation**: `Agent/MessagePack/MessagePackStringCache.cs` — `readonly struct CachedBytes` +keyed by string value, cached per-thread via `[ThreadStatic]`. + +**When to use**: Any string that is serialized on every span but rarely changes. + +--- + +## Category 3: Value Types + +### 3.1 `readonly struct` + +**Avoids**: Heap allocation for small, short-lived data containers. + +**Examples**: +- `Agent/StatsAggregationKey.cs:10` — `readonly struct StatsAggregationKey` +- `Iast/Vulnerability.cs:12` — `readonly struct Vulnerability` +- `ClrProfiler/CallTarget/CallTargetState.cs:19` — `readonly struct CallTargetState` +- `Agent/MessagePack/SpanModel.cs:13` — `readonly struct SpanModel` +- `Agent/MessagePack/TraceChunkModel.cs:21` — `readonly struct TraceChunkModel` + +**When to use**: Small data carriers (under ~64 bytes), keys, intermediate results, return +types from hot methods. The `readonly` modifier prevents defensive copies. + +### 3.2 `ref struct` + +**Avoids**: Any heap allocation — `ref struct` cannot escape to the heap. + +**Examples**: +- `Tagging/TagItem.cs:10` — `readonly ref struct TagItem` (carries `ReadOnlySpan`) +- `Util/SpanCharSplitter.cs:13` — `readonly ref struct SpanCharSplitter` +- `ClrProfiler/CallTarget/CallTargetReturn.cs:18` — `readonly ref struct CallTargetReturn` +- `Util/FixedSizeArrayPool.cs:99` — `ref struct ArrayPoolItem` (RAII dispose pattern) +- `Iast/Propagation/StringModuleImpl.cs:592` — `ref struct StringConcatParams` + +**When to use**: Types that hold `Span`, temporary enumerators, RAII wrappers that must +not escape the calling method. + +### 3.3 Struct Enumerators (Allocation-Free Iteration) + +**Avoids**: Boxing a struct enumerator to `IEnumerator` when used via `foreach`. + +**Examples**: +- `Agent/SpanCollection.cs:207` — `public struct Enumerator : IEnumerator` +- `Util/SpanCharSplitter.cs:30` — `ref struct SpanSplitEnumerator` (zero-alloc string splitting) +- `Activity/Helpers/AllocationFreeEnumerator.cs` — `DynamicMethod`-based approach to force + struct enumerator usage on arbitrary `IEnumerable` + +**Pattern**: Expose `GetEnumerator()` returning a concrete struct type. C#'s `foreach` will +use the struct directly without boxing: +```csharp +internal readonly struct MyCollection : IEnumerable +{ + public Enumerator GetEnumerator() => new Enumerator(this); + + public struct Enumerator : IEnumerator + { + // ... + } +} +``` + +### 3.4 Generic Struct Constraints (Avoid Boxing) + +**Avoids**: Boxing value types when calling interface methods. + +**Examples**: +- `Propagators/SpanContextPropagator.cs:73` — `where TCarrierSetter : struct, ICarrierSetter` +- `Propagators/W3CTraceContextPropagator.cs:122` — `where TCarrierSetter : struct, ICarrierSetter` +- `Activity/DiagnosticSourceEventListener.cs:79` — `where T : struct, IActivity` +- `Baggage.cs:450` — `where T : struct, ICancellableObserver>` + +**Pattern**: Define an interface, implement it as a `readonly struct`, and constrain the +generic parameter: +```csharp +internal interface ICarrierSetter +{ + void Set(TCarrier carrier, string key, string value); +} + +internal readonly struct HttpRequestCarrierSetter : ICarrierSetter { ... } + +// The JIT devirtualizes calls — no boxing, no interface dispatch overhead +internal void Inject(TCarrier carrier, TCarrierSetter setter) + where TCarrierSetter : struct, ICarrierSetter +``` + +--- + +## Category 4: String Optimization + +### 4.1 `string.Create` with `stackalloc` + +**Avoids**: Intermediate string allocations when formatting composite strings. + +**Examples**: +- `Propagators/W3CTraceContextPropagator.cs:149`: + ```csharp + return string.Create(null, stackalloc char[128], $"00-{context.RawTraceId}-{context.RawSpanId}-{sampled}"); + ``` +- `Propagators/B3SingleHeaderContextPropagator.cs:158` +- `ServiceFabric/ServiceRemotingHelpers.cs:195` + +**When to use**: Formatting short strings from known components. + +**Caveats**: Requires .NET 6+ (`string.Create` with `IFormatProvider?` overload). + +### 4.2 `ReadOnlySpan` / `.AsSpan()` for String Slicing + +**Avoids**: `string.Substring()` allocating a new string for intermediate parsing. + +**Examples**: ~181 occurrences across 42 non-vendor files: +- `Propagators/W3CTraceContextPropagator.cs` — parsing trace headers +- `Debugger/ExceptionAutoInstrumentation/ExceptionNormalizer.cs` — stack frame parsing +- `Propagators/B3SingleHeaderContextPropagator.cs` — header parsing + +**Pattern**: +```csharp +// Before (allocates): +var part = header.Substring(start, length); + +// After (zero-alloc): +ReadOnlySpan part = header.AsSpan(start, length); +``` + +### 4.3 `string.Split()` Replacement with `SpanCharSplitter` + +**Avoids**: `string.Split()` allocates both a `string[]` and a new `string` for each segment. + +**Implementation**: `Util/SpanCharSplitter.cs` — a `readonly ref struct` with a `ref struct` +enumerator that yields `ReadOnlySpan` slices without any allocation. + +**Examples**: +- `Util/SpanCharSplitter.cs:13` — the implementation +- Usage pattern via `foreach`: + ```csharp + // Before (allocates string[] + individual strings): + foreach (var part in input.Split(',')) + { + Process(part); + } + + // After (zero-alloc on NETCOREAPP): + foreach (var part in new SpanCharSplitter(input, ',', int.MaxValue)) + { + Process(part.AsSpan()); + } + ``` + +**Caveats**: Only available on `NETCOREAPP` (uses `ref struct` and `Span`). For .NET +Framework, use `IndexOf`-based manual parsing or accept the `Split()` allocation. + +### 4.4 Pre-Computed Static Strings and Arrays + +**Avoids**: Repeated string construction or array creation for constant values. + +**Examples**: +- `RuntimeMetrics/RuntimeEventListener.cs:36-37` — `static readonly string[] CompactingGcTags` +- `SpanContext.cs:26` — `static readonly string[] KeyNames` +- `PlatformHelpers/ServiceFabric.cs:14-22` — static readonly env var lookups at startup + +--- + +## Category 5: JIT Hints + +### 5.1 `[MethodImpl(MethodImplOptions.AggressiveInlining)]` + +**Avoids**: Method call overhead; enables further JIT optimizations (struct promotion, +dead-code elimination). + +~690 occurrences across hot paths: +- `Util/FixedSizeArrayPool.cs:44` — on `Get()` +- `Util/SpanCharSplitter.cs:19,27` — on constructor and `GetEnumerator()` +- `ClrProfiler/CallTarget/CallTargetReturn.cs`, `CallTargetState.cs` + +**When to use**: Small, frequently-called methods (1-3 lines). Especially important for +`readonly struct` methods to enable the JIT to promote them to registers. + +### 5.2 `ThrowHelper` with `[MethodImpl(NoInlining)]` + +**Avoids**: The JIT inlining exception-throwing code into the caller, which bloats the hot +path and may prevent the caller from being inlined. + +**Implementation**: `Util/ThrowHelper.cs` — centralized `[DoesNotReturn]` throw methods. + +**Examples**: +- `Util/ThrowHelper.cs:16` — `[MethodImpl(MethodImplOptions.NoInlining)]` on all throw methods +- `Util/UnmanagedMemoryPool.cs:212` — local `ThrowObjectDisposedException()` with `[NoInlining]` + +**Pattern**: +```csharp +// Before (prevents caller inlining): +if (arg is null) throw new ArgumentNullException(nameof(arg)); + +// After (keeps hot path small): +if (arg is null) ThrowHelper.ThrowArgumentNullException(nameof(arg)); +``` + +--- + +## Category 6: API Design for Zero Allocation + +### 6.1 Generic Log Overloads (Avoid `params` + Boxing) + +**Avoids**: `params object[]` array allocation AND boxing of value-type arguments. + +**Implementation**: `Logging/Internal/DatadogSerilogLogger.cs:211-284` — typed overloads +for 1-5 arguments using `FixedSizeArrayPool`. + +**Pattern**: +```csharp +// Provide typed overloads instead of params: +void Write(LogEventLevel level, string template, T property) { ... } +void Write(LogEventLevel level, string template, T0 p0, T1 p1) { ... } + +// Each overload: +// 1. Checks IsEnabled first (avoids work entirely if disabled) +// 2. Uses FixedSizeArrayPool to get a reusable array +// 3. Boxes only after confirming the log will actually be written +``` + +### 6.2 Static / Cached Delegates + +**Avoids**: Allocating a new delegate object on each call. + +**Examples**: +- `AspNet/SharedItems.cs:17-18` — `static readonly Func, Scope> Pop` +- `Debugger/DebuggerManager.cs:33-34` — `static readonly Func ServiceNameProvider` +- `RuntimeMetrics/RuntimeMetricsWriter.cs:34` — `static readonly Func<...> InitializeListenerFunc` +- `Iast/IastModule.cs:65` — `static readonly Func Always` + +**When to use**: Any delegate passed to a method that is called frequently. The `static` +keyword on lambdas (C# 9+) prevents accidental captures. + +### 6.3 `ValueTask` / `ValueTask` for Hot Async Paths + +**Avoids**: `Task` / `Task` heap allocation when the async method completes synchronously +(which is the common case for cached results, already-available data, etc.). + +**When to use**: Async methods on hot paths that frequently complete synchronously. Common in +I/O code with caching layers, or methods that only go async on cache miss. + +**Pattern**: +```csharp +#if NETCOREAPP3_1_OR_GREATER +// Return ValueTask when synchronous completion is common: +internal ValueTask GetCachedValueAsync() +{ + if (_cache.TryGetValue(key, out var value)) + return new ValueTask(value); // no Task allocation + + return GetValueSlowAsync(); // only allocates Task on cache miss +} +#else +internal Task GetCachedValueAsync() +{ + if (_cache.TryGetValue(key, out var value)) + return Task.FromResult(value); + + return GetValueSlowAsync(); +} +#endif +``` + +**Caveats**: +- `ValueTask` is available from .NET Core 2.1+ but pooled async builders require + `NETCOREAPP3_1_OR_GREATER` — use `#if` guards for .NET Framework +- `ValueTask` can only be awaited once (unlike `Task`). Do not cache or await multiple times +- Prefer `Task` when the result is always async (network calls without caching) + +--- + +## Category 7: Unsafe / Low-Level + +### 7.1 `Unsafe.As` / `Unsafe.ReadUnaligned` + +**Avoids**: Memory copies when reinterpreting data. + +**Examples**: +- `Ci/Coverage/Util/FileBitmap.cs:190+` — SIMD-optimized bitmap operations using + `Unsafe.ReadUnaligned>`, `Vector256`, `Vector128`, `ulong`, `uint` + +**When to use**: Performance-critical data processing where the type system gets in the way. +Requires careful correctness review. + +### 7.2 `HexConverter` with Unsafe Pointers + +**Avoids**: Intermediate allocations in hex encoding. + +**Implementation**: `Util/HexConverter.cs:158`: +```csharp +return string.Create(bytes.Length * 2, (RosPtr: (IntPtr)(&bytes), casing), static (chars, args) => { ... }); +``` + +Uses `string.Create` with a state tuple containing an unsafe pointer to avoid copying the +input span. diff --git a/TODO.md b/TODO.md new file mode 100644 index 000000000000..9bc348c9b99a --- /dev/null +++ b/TODO.md @@ -0,0 +1,60 @@ +# TODO + +## Allocation Analyzers + +New Roslyn analyzers to detect heap-allocation anti-patterns at compile time. Add to `tracer/src/Datadog.Trace.Tools.Analyzers/` (analyzer) and `tracer/src/Datadog.Trace.Tools.Analyzers.CodeFixes/` (code fix). Both projects target `netstandard2.0`. Use `DDALLOC` prefix for diagnostic IDs. Follow existing conventions (see `LogAnalyzer/` or `SealedAnalyzer/` for examples). + +- [ ] **DDALLOC001: `ToString()` on numeric types in log calls** + - Detect calls to `IDatadogLogger` methods (`Debug`, `Information`, `Warning`, `Error`, `ErrorSkipTelemetry`) where any argument has `.ToString()` called on a numeric type (`int`, `long`, `double`, etc.) + - The generic log overloads (`Log.Debug(...)`) handle formatting without allocating a string — the `.ToString()` is unnecessary and allocates + - Code fix: remove the `.ToString()` call and pass the value directly; if the log call uses explicit generic type args (e.g., `Log.Debug(...)`), update the type arg to match the numeric type + - Example: `Log.Debug("attempt {Attempt}", (attempt + 1).ToString())` → `Log.Debug("attempt {Attempt}", attempt + 1)` + - Severity: Warning + - Extends the existing `LogAnalyzer` infrastructure in `tracer/src/Datadog.Trace.Tools.Analyzers/LogAnalyzer/` + - [ ] Remove AP-2 from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working + +- [ ] **DDALLOC002: Missing `JsonArrayPool` on Newtonsoft.Json readers/writers** + - Detect `new JsonTextReader(...)` and `new JsonTextWriter(...)` object creations (types: `Newtonsoft.Json.JsonTextReader`, `Newtonsoft.Json.JsonTextWriter`) where the `ArrayPool` property is not set to `JsonArrayPool.Shared` in an object initializer + - Without `ArrayPool`, Newtonsoft.Json allocates internal `char[]` buffers on every read/write + - Code fix: add `{ ArrayPool = JsonArrayPool.Shared }` object initializer (or append to existing initializer) + - `JsonArrayPool` lives at `Datadog.Trace.Util.Json.JsonArrayPool` + - Example: `new JsonTextReader(sr)` → `new JsonTextReader(sr) { ArrayPool = JsonArrayPool.Shared }` + - Severity: Warning + - [ ] Remove AP-11 from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working + +- [ ] **DDALLOC003: `new StringBuilder()` instead of `StringBuilderCache`** + - Detect `new StringBuilder()` and `new StringBuilder(int)` object creations in tracer code + - `StringBuilderCache` (`Datadog.Trace.Util.StringBuilderCache`) uses a `[ThreadStatic]` cached instance — avoids allocating a new `StringBuilder` per call + - Code fix: replace `new StringBuilder()` with `StringBuilderCache.Acquire()` and ensure the result is consumed via `StringBuilderCache.GetStringAndRelease(sb)`. This fix is complex (needs to track usage to insert the release call), so a diagnostic-only analyzer (no code fix) is acceptable as a first pass + - Suppress when: capacity > 360 (exceeds `StringBuilderCache` max), or inside a method that already calls `StringBuilderCache.Acquire()` (not nestable on the same thread) + - Severity: Info (suggestion) + - [ ] Remove AP-12 from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working + +- [ ] **DDALLOC004: `Enum.HasFlag()` boxing** + - Detect calls to `.HasFlag()` on enum-typed expressions + - On .NET Framework and pre-.NET 7 runtimes, `Enum.HasFlag()` boxes both operands. Since the tracer targets `netstandard2.0`/`net462`+, this is always relevant + - Code fix: rewrite `flags.HasFlag(MyEnum.Value)` → `(flags & MyEnum.Value) != 0`. For multi-flag checks like `flags.HasFlag(MyEnum.A | MyEnum.B)`, rewrite to `(flags & (MyEnum.A | MyEnum.B)) == (MyEnum.A | MyEnum.B)` + - Severity: Warning + - [ ] Remove AP-9 (`HasFlag` portion) from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working + +- [ ] **DDALLOC005: `Enum.ToString()` allocates** + - Detect `.ToString()` calls on enum-typed expressions (use semantic model: check if receiver type's `TypeKind == TypeKind.Enum`) + - `Enum.ToString()` boxes the value and allocates a string via reflection on all runtimes + - Code fix is hard to auto-generate (would need a switch expression over all enum members), so diagnostic-only is fine. Message should suggest using a switch expression or cached dictionary lookup + - Severity: Info (suggestion) — lower priority since the fix is manual + - [ ] Remove AP-9 (`ToString` portion) from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working + +- [ ] **DDALLOC006: Capturing lambdas in `Task.Run` / `ContinueWith`** + - Detect lambdas passed to `Task.Run(...)`, `Task.Factory.StartNew(...)`, and `.ContinueWith(...)` that are not marked `static` (C# 9+) and capture variables from the enclosing scope + - Capturing lambdas allocate a compiler-generated display class + a delegate object. The `state` parameter overloads avoid this + - Use semantic model's data flow analysis (`AnalyzeDataFlow`) on the lambda to check for captured variables, or check for non-`static` lambdas that reference identifiers from enclosing scope + - No auto-fix — restructuring to use `state` parameter varies too much. Diagnostic message should suggest: (1) use `static` lambda if no captures needed, (2) use `Task.Factory.StartNew(static state => ..., stateObj)` overload to pass state without closure + - Severity: Info (suggestion) + - [ ] Remove AP-14 from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working + +- [ ] **DDALLOC007: `throw new` in `[AggressiveInlining]` methods** + - Detect `throw new XxxException(...)` statements inside methods decorated with `[MethodImpl(MethodImplOptions.AggressiveInlining)]` + - `throw` prevents the JIT from inlining the method, defeating the purpose of `[AggressiveInlining]`. The throw should be moved to a separate `[MethodImpl(MethodImplOptions.NoInlining)]` helper (see `ThrowHelper` pattern at `Util/ThrowHelper.cs`) + - No auto-fix — the developer needs to decide the ThrowHelper method name/location. Diagnostic message should suggest extracting to a `ThrowHelper` method with `[DoesNotReturn]` and `[NoInlining]` + - Severity: Warning + - [ ] Remove AP-13 from `.claude/skills/find-allocation-opportunities/references/anti-patterns.md` once analyzer is working