From 2a1a7847aab8b1faa29041507cec8b25ca0d6d1b Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:43:32 +1100 Subject: [PATCH 1/5] fixes --- .claude/skills/submit-pr/SKILL.md | 2 +- .../lsp-profiler-dotnet/tests/dotnet_e2e.rs | 5 ++++- .../tests/dotnet_fsharp_e2e.rs | 5 ++++- .../lsp-profiler-dotnet/tests/support/mod.rs | 20 +++++++++++++++++++ .../LspProfiler.DotNet.Engine/Symbolizer.cs | 8 +++++--- 5 files changed, 34 insertions(+), 6 deletions(-) diff --git a/.claude/skills/submit-pr/SKILL.md b/.claude/skills/submit-pr/SKILL.md index 63d336d..dd9b192 100644 --- a/.claude/skills/submit-pr/SKILL.md +++ b/.claude/skills/submit-pr/SKILL.md @@ -1,7 +1,7 @@ --- name: submit-pr description: Creates a pull request with a well-structured description after verifying CI passes. Use when the user asks to submit, create, or open a pull request. -disable-model-invocation: true +disable-model-invocation: false --- diff --git a/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs b/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs index abb0765..736de88 100644 --- a/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs +++ b/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs @@ -24,11 +24,13 @@ use std::time::Duration; use lsp_profiler::wire::methods; use lsp_profiler::ProfilerService; use serde_json::{json, Value}; -use support::{call, completed, engine, service, Fixture}; +use support::{assert_only_busy_threads_count, call, completed, engine, service, Fixture}; const FIXTURE: &str = "HotLeakApp"; const FIXTURE_TYPE: &str = "HotLeakApp.LeakedPayload"; const HOT_FUNCTION: &str = "HotLeakApp.Hot.Spin"; +/// `Program.Main`, which only ever waits on stdin. +const ENTRY_POINT: &str = "HotLeakApp.Program.Main("; #[tokio::test(flavor = "multi_thread")] async fn the_fixture_is_recognised_as_an_attachable_dotnet_process() { @@ -104,6 +106,7 @@ async fn cpu_profiling_finds_the_hot_function_and_its_lines() { && (29..=32).contains(&l["line"].as_i64().unwrap_or(0))), "hot line inside Hot.Spin expected: {report}" ); + assert_only_busy_threads_count(&report, ENTRY_POINT, HOT_FUNCTION); for key in ["speedscope_path", "flamegraph_path", "cpuprofile_path"] { let path = report["artifacts"][key] .as_str() diff --git a/crates/lsp-profiler-dotnet/tests/dotnet_fsharp_e2e.rs b/crates/lsp-profiler-dotnet/tests/dotnet_fsharp_e2e.rs index 566f1aa..669e737 100644 --- a/crates/lsp-profiler-dotnet/tests/dotnet_fsharp_e2e.rs +++ b/crates/lsp-profiler-dotnet/tests/dotnet_fsharp_e2e.rs @@ -23,13 +23,15 @@ use std::time::Duration; use lsp_profiler::wire::methods; use serde_json::{json, Value}; -use support::{call, completed, service, Fixture}; +use support::{assert_only_busy_threads_count, call, completed, service, Fixture}; const FIXTURE: &str = "HotLeakAppFs"; const RECORD_TYPE: &str = "HotLeakAppFs.LeakedPayload"; const UNION_CASE_TYPE: &str = "HotLeakAppFs.PayloadKind+Small"; const GENERIC_RECORD_TYPE: &str = "HotLeakAppFs.Tagged`1[System.Int32]"; const HOT_FUNCTION: &str = "HotLeakAppFs.Hot.spin"; +/// `Program.main`, which only ever waits on stdin. +const ENTRY_POINT: &str = "HotLeakAppFs.Program.main("; /// `items.Add { Data = …; Label = …; Kind = kind }` in `LeakCache.grow`. const RECORD_CONSTRUCTED_AT: i64 = 23; /// `let kind = Small items.Count` in `LeakCache.grow`. @@ -89,6 +91,7 @@ async fn cpu_profiling_finds_the_fsharp_hot_function_and_its_lines() { .any(|l| in_program_fs(l) && (26..=31).contains(&l["line"].as_i64().unwrap_or(0))), "hot line inside Hot.spin expected: {report}" ); + assert_only_busy_threads_count(&report, ENTRY_POINT, HOT_FUNCTION); service.shutdown().await; } diff --git a/crates/lsp-profiler-dotnet/tests/support/mod.rs b/crates/lsp-profiler-dotnet/tests/support/mod.rs index 909caee..bf4d5d9 100644 --- a/crates/lsp-profiler-dotnet/tests/support/mod.rs +++ b/crates/lsp-profiler-dotnet/tests/support/mod.rs @@ -108,6 +108,26 @@ pub async fn call(service: &ProfilerService, method: &str, params: Value) -> Val .unwrap_or_else(|error| panic!("{method} failed: {error}")) } +/// [PROF-AGGREGATION-LOGIC] and [PROF-DOTNET-CPU-SYMBOLS]: only a thread +/// executing managed code counts. A fixture's main thread waits on stdin for +/// the whole session, so its entry point is never hot, and the worker's hot +/// function holds most of the share. Counting the idle thread caps that share +/// at half, since the runtime samples both threads every tick. +pub fn assert_only_busy_threads_count(report: &Value, entry_point: &str, hot_function: &str) { + let functions = report["hot_functions"].as_array().expect("hot_functions"); + let named = |prefix: &str| { + functions + .iter() + .find(|f| f["name"].as_str().is_some_and(|n| n.starts_with(prefix))) + }; + assert!( + named(entry_point).is_none(), + "{entry_point} only runs on the idle main thread: {report}" + ); + let share = named(hot_function).map_or(0.0, |f| f["percentage"].as_f64().unwrap_or(0.0)); + assert!(share > 50.0, "{hot_function} owns the busy thread, {share}%: {report}"); +} + /// The result of a memory step the direct backend completed. pub fn completed(step: &Value) -> &Value { step.get("result") diff --git a/dotnet/LspProfiler.DotNet.Engine/Symbolizer.cs b/dotnet/LspProfiler.DotNet.Engine/Symbolizer.cs index a1a423d..dbf1e58 100644 --- a/dotnet/LspProfiler.DotNet.Engine/Symbolizer.cs +++ b/dotnet/LspProfiler.DotNet.Engine/Symbolizer.cs @@ -9,6 +9,7 @@ using Microsoft.Diagnostics.Symbols; using Microsoft.Diagnostics.Tracing; using Microsoft.Diagnostics.Tracing.Etlx; +using Microsoft.Diagnostics.Tracing.EventPipe; namespace LspProfiler.DotNet.Engine; @@ -17,8 +18,6 @@ internal sealed class Symbolizer(bool includeNative) private const string SampleProvider = "Microsoft-DotNETCore-SampleProfiler"; private const int TicksPerMessage = 256; private const double MaxTickWeightMs = 10.0; - /// The sample-type payload value for a thread executing managed code. - private const int ManagedSample = 2; private readonly FrameTable _frames = new(includeNative); @@ -110,8 +109,11 @@ private void Flush() } } + // A thread waiting in native code (a lock, a sleep, a console read) is + // sampled as External and is idle. TraceLog parses every sample as a + // ClrThreadSampleTraceData, whose Type is an enum, never a boxed int. private static bool IsManaged(TraceEvent sample) => - sample.PayloadByName("Type") is int type ? type == ManagedSample : true; + sample is ClrThreadSampleTraceData { Type: ClrThreadSampleType.Managed }; } } From 8849a62892e2a040225fc8165876bf517ae8b653 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:47:24 +1100 Subject: [PATCH 2/5] Reflow the busy-thread assertion for rustfmt --- crates/lsp-profiler-dotnet/tests/support/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/lsp-profiler-dotnet/tests/support/mod.rs b/crates/lsp-profiler-dotnet/tests/support/mod.rs index bf4d5d9..ed2f939 100644 --- a/crates/lsp-profiler-dotnet/tests/support/mod.rs +++ b/crates/lsp-profiler-dotnet/tests/support/mod.rs @@ -125,7 +125,10 @@ pub fn assert_only_busy_threads_count(report: &Value, entry_point: &str, hot_fun "{entry_point} only runs on the idle main thread: {report}" ); let share = named(hot_function).map_or(0.0, |f| f["percentage"].as_f64().unwrap_or(0.0)); - assert!(share > 50.0, "{hot_function} owns the busy thread, {share}%: {report}"); + assert!( + share > 50.0, + "{hot_function} owns the busy thread, {share}%: {report}" + ); } /// The result of a memory step the direct backend completed. From d3e249a51a970c8d30bbd1a76d796edb4d1e95cb Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:51:18 +1100 Subject: [PATCH 3/5] Profile F# memory correctly: an array has no source location, and each diff replaces the last diff's leak badges - SourceLocator: TypeNames.Definition cut `Order[]` at its bracket, so a ResizeArray's backing array, which survives Clear(), was anchored at the line that builds an Order and kept painting it after the last Order died. An array is the runtime's own type and has no location ([PROF-DOTNET-SOURCE]). - memory-decorations: applyLeakDecorations never cleared the previous diff's badges, and badges shared per-colour decoration types with the allocation track. A LOW badge survived beside the next MEDIUM one, a flat diff left every badge standing, and a LOW badge overwrote allocation paint of the same purple. The allocation track and the leak badges are now separate layers; each diff replaces only the badges. --- docs/specs/LSP-PROFILER-SPEC.md | 2 +- .../SourceLocator.cs | 15 +- .../lsp-profiler-ui/src/memory-decorations.ts | 273 ++++++++---------- 3 files changed, 139 insertions(+), 151 deletions(-) diff --git a/docs/specs/LSP-PROFILER-SPEC.md b/docs/specs/LSP-PROFILER-SPEC.md index e5bf41a..2b7c007 100644 --- a/docs/specs/LSP-PROFILER-SPEC.md +++ b/docs/specs/LSP-PROFILER-SPEC.md @@ -738,7 +738,7 @@ Every member F# generates for a record, a union or a union case has only hidden - a `newobj` of one of the type's constructors (records, closures, anonymous records), including a generic type's constructor reached through a member reference; - a call to the static `New` factory F# emits on a union. It constructs the nested `Case` class, or the union itself for a single-case union. -Only methods with visible sequence points are searched, so the generated factories themselves never count. Types without a PDB, such as the framework's, have no location and appear by name only. +Only methods with visible sequence points are searched, so the generated factories themselves never count. Types without a PDB, such as the framework's, have no location and appear by name only. An array (`Element[]`, `Element[,]`) is the runtime's own type, built wherever a collection grows, so it has no location whatever its element type: a `ResizeArray` keeps its `Order[]` after `Clear()`, and that array must never paint the line that builds an `Order`. ### [PROF-DOTNET-MEMORY] Memory steps diff --git a/dotnet/LspProfiler.DotNet.Engine/SourceLocator.cs b/dotnet/LspProfiler.DotNet.Engine/SourceLocator.cs index 3b8fbe4..8808a3f 100644 --- a/dotnet/LspProfiler.DotNet.Engine/SourceLocator.cs +++ b/dotnet/LspProfiler.DotNet.Engine/SourceLocator.cs @@ -5,7 +5,9 @@ // memory finding about the type. A type with no visible point of its own, such // as an F# record or union, anchors where the assembly's code constructs it // (ConstructionSites). Types from assemblies without a PDB (the framework) have -// no location and are reported by name alone. +// no location and are reported by name alone. Neither has an array: `Order[]` +// is the runtime's type, grown wherever a collection resizes, so anchoring it at +// Order's line would paint that line long after the last Order died. using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; using System.Reflection.PortableExecutable; @@ -23,7 +25,9 @@ internal sealed class SourceLocator : IDisposable { return known; } - var located = type.Module is { } path && File.Exists(path) ? Module(path)?.Locate(TypeNames.Definition(type.Name)) : null; + var located = type.Module is { } path && File.Exists(path) && !TypeNames.IsArray(type.Name) + ? Module(path)?.Locate(TypeNames.Definition(type.Name)) + : null; _located[type] = located; return located; } @@ -140,6 +144,13 @@ public static IReadOnlyList Definition(string runtimeName) return bare.Split('+'); } + /// An array, `Element[]` or `Element[,]`: its last bracket holds only commas, never a type argument. + public static bool IsArray(string runtimeName) + { + var open = runtimeName.LastIndexOf('['); + return open >= 0 && runtimeName.EndsWith(']') && !runtimeName.AsSpan(open + 1, runtimeName.Length - open - 2).ContainsAnyExcept(','); + } + public static (string Namespace, string Name) Split(string qualified) { var dot = qualified.LastIndexOf('.'); diff --git a/packages/lsp-profiler-ui/src/memory-decorations.ts b/packages/lsp-profiler-ui/src/memory-decorations.ts index 042cdb5..3ac61ae 100644 --- a/packages/lsp-profiler-ui/src/memory-decorations.ts +++ b/packages/lsp-profiler-ui/src/memory-decorations.ts @@ -68,37 +68,118 @@ export interface MemoryDiffResult { suspectedLeaks: SuspectedLeak[]; } -// ── Applied-decoration ledger ───────────────────────────────────────────── +// ── Layers ──────────────────────────────────────────────────────────────── // Implements [PROF-UI-HEATMAP] (memory track) observability — same e2e // seam as the CPU heat map, since `setDecorations` is write-only. -let appliedMemDecorations: AppliedDecoration[] = []; +/** One line to paint: where, in which colour, saying what. */ +interface PaintedLine { + file: string; + line: number; + color: string; + text: string; +} -/** The memory/leak decorations currently applied across visible editors. */ -export function appliedMemoryDecorations(): readonly AppliedDecoration[] { - return appliedMemDecorations; +/** + * One layer of the memory track, with its own decoration types and its own + * ledger. The allocation track and the leak badges are separate layers: a diff + * replaces the last diff's badges and never touches the allocations, even where + * a badge and an allocation share a colour. + */ +class MemoryLayer { + private readonly types = new Map(); + private applied: AppliedDecoration[] = []; + + get decorations(): readonly AppliedDecoration[] { return this.applied; } + + /** Replace this layer's paint with `lines`, on every visible editor. */ + paint(lines: readonly PaintedLine[]): void { + this.clear(); + // Keyed, not compared raw: tracemalloc reports the interpreter's own + // filename and the editor reports what VS Code resolved. On win32 those + // differ in drive-letter case and 8.3 short components, so a raw compare + // matches nothing and the memory track silently paints nothing at all + // ([PROF-TESTING]). + const keyOf = pathKeyer(); + const byFile = new Map(); + for (const line of lines) { + const key = keyOf(line.file); + byFile.set(key, [...(byFile.get(key) ?? []), line]); + } + for (const editor of vscode.window.visibleTextEditors) { + const painted = byFile.get(keyOf(editor.document.uri.fsPath)); + if (painted !== undefined) { this.paintEditor(editor, painted); } + } + } + + clear(): void { + for (const decorationType of this.types.values()) { + for (const editor of vscode.window.visibleTextEditors) { + editor.setDecorations(decorationType, []); + } + } + this.applied = []; + } + + dispose(): void { + for (const decorationType of this.types.values()) { + decorationType.dispose(); + } + this.types.clear(); + this.applied = []; + } + + private paintEditor(editor: vscode.TextEditor, lines: readonly PaintedLine[]): void { + const optionsByColor = new Map(); + for (const line of lines) { + const option = lineOption(editor.document, line); + if (option !== undefined) { + optionsByColor.set(line.color, [...(optionsByColor.get(line.color) ?? []), option]); + } + } + for (const [color, options] of optionsByColor) { + editor.setDecorations(this.typeFor(color), options); + } + recordApplied(this.applied, editor.document.uri.fsPath, optionsByColor); + } + + private typeFor(color: string): vscode.TextEditorDecorationType { + const existing = this.types.get(color); + if (existing !== undefined) { return existing; } + const created = vscode.window.createTextEditorDecorationType({ + overviewRulerColor: color, + overviewRulerLane: vscode.OverviewRulerLane.Right, + isWholeLine: false, + after: { + margin: "0 0 0 2em", + fontStyle: "normal", + fontWeight: "500", + }, + }); + this.types.set(color, created); + return created; + } } -// ── Decoration types ────────────────────────────────────────────────────── - -const memDecorationTypes = new Map(); - -function getMemDecorationTypeForColor(color: string): vscode.TextEditorDecorationType { - let existing = memDecorationTypes.get(color); - if (existing !== undefined) { return existing; } - - existing = vscode.window.createTextEditorDecorationType({ - overviewRulerColor: color, - overviewRulerLane: vscode.OverviewRulerLane.Right, - isWholeLine: false, - after: { - margin: "0 0 0 2em", - fontStyle: "normal", - fontWeight: "500", - }, - }); - memDecorationTypes.set(color, existing); - return existing; +/** The whole-line decoration for `line`, or nothing when the document has no such line. */ +function lineOption(document: vscode.TextDocument, line: PaintedLine): vscode.DecorationOptions | undefined { + const lineIdx = line.line - 1; + if (lineIdx < 0 || lineIdx >= document.lineCount) { return undefined; } + return { + range: new vscode.Range( + new vscode.Position(lineIdx, 0), + new vscode.Position(lineIdx, document.lineAt(lineIdx).text.length), + ), + renderOptions: { after: { contentText: line.text, color: line.color } }, + }; +} + +const allocationLayer = new MemoryLayer(); +const leakLayer = new MemoryLayer(); + +/** The memory/leak decorations currently applied across visible editors. */ +export function appliedMemoryDecorations(): readonly AppliedDecoration[] { + return [...allocationLayer.decorations, ...leakLayer.decorations]; } // ── Formatting ──────────────────────────────────────────────────────────── @@ -151,141 +232,37 @@ function confidenceBadge(confidence: LeakConfidence): string { // ── Public API ──────────────────────────────────────────────────────────── -/** Apply memory allocation decorations to visible editors. */ +/** Apply memory allocation decorations to visible editors; a new snapshot drops the last diff's badges. */ export function applyMemoryDecorations(result: MemorySnapshotResult): void { - clearMemoryDecorations(); - - // Keyed, not compared raw: tracemalloc reports the interpreter's own - // filename and the editor reports what VS Code resolved. On win32 those - // differ in drive-letter case and 8.3 short components, so a raw compare - // matches nothing and the memory track silently paints nothing at all - // ([PROF-TESTING]). - const keyOf = pathKeyer(); - const allocsByFile = new Map(); - for (const alloc of result.topAllocations) { - const key = keyOf(alloc.file); - const existing = allocsByFile.get(key); - if (existing !== undefined) { - existing.push(alloc); - } else { - allocsByFile.set(key, [alloc]); - } - } - - for (const editor of vscode.window.visibleTextEditors) { - const filePath = editor.document.uri.fsPath; - const allocs = allocsByFile.get(keyOf(filePath)); - if (allocs === undefined) { continue; } - - const optionsByColor = new Map(); - - for (const alloc of allocs) { - const color = memColor(alloc.size); - const text = ` \u2588\u2588\u2588\u2588 ${formatBytes(alloc.size)} allocated (${alloc.count} objects)`; - const lineIdx = alloc.line - 1; - if (lineIdx < 0 || lineIdx >= editor.document.lineCount) { continue; } - - const range = new vscode.Range( - new vscode.Position(lineIdx, 0), - new vscode.Position(lineIdx, editor.document.lineAt(lineIdx).text.length), - ); - - const option: vscode.DecorationOptions = { - range, - renderOptions: { after: { contentText: text, color } }, - }; - - const existing = optionsByColor.get(color); - if (existing !== undefined) { - existing.push(option); - } else { - optionsByColor.set(color, [option]); - } - } - - for (const [color, options] of optionsByColor) { - editor.setDecorations(getMemDecorationTypeForColor(color), options); - } - recordApplied(appliedMemDecorations, filePath, optionsByColor); - } - + leakLayer.clear(); + allocationLayer.paint(result.topAllocations.map((alloc) => ({ + file: alloc.file, + line: alloc.line, + color: memColor(alloc.size), + text: ` \u2588\u2588\u2588\u2588 ${formatBytes(alloc.size)} allocated (${alloc.count} objects)`, + }))); Logger.info(`Memory decorations applied: ${result.topAllocations.length} allocations`); } -/** Apply leak indicator decorations with confidence badges to visible editors. */ +/** Apply leak indicator decorations with confidence badges, replacing the last diff's. */ export function applyLeakDecorations(result: MemoryDiffResult): void { - // Keyed for the same reason as the allocation track above — a raw compare - // never matches on win32 ([PROF-TESTING]). - const keyOf = pathKeyer(); - const leaksByFile = new Map(); - for (const leak of result.suspectedLeaks) { - const key = keyOf(leak.file); - const existing = leaksByFile.get(key); - if (existing !== undefined) { - existing.push(leak); - } else { - leaksByFile.set(key, [leak]); - } - } - - for (const editor of vscode.window.visibleTextEditors) { - const filePath = editor.document.uri.fsPath; - const leaks = leaksByFile.get(keyOf(filePath)); - if (leaks === undefined) { continue; } - - const optionsByColor = new Map(); - - for (const leak of leaks) { - const color = leakColor(leak.confidence); - const badge = confidenceBadge(leak.confidence); - const growth = formatBytes(Math.abs(leak.sizeGrowth)); - const text = ` ${badge} +${growth} leak (${leak.countGrowth} objects)`; - - const lineIdx = leak.line - 1; - if (lineIdx < 0 || lineIdx >= editor.document.lineCount) { continue; } - - const range = new vscode.Range( - new vscode.Position(lineIdx, 0), - new vscode.Position(lineIdx, editor.document.lineAt(lineIdx).text.length), - ); - - const option: vscode.DecorationOptions = { - range, - renderOptions: { after: { contentText: text, color } }, - }; - - const existing = optionsByColor.get(color); - if (existing !== undefined) { - existing.push(option); - } else { - optionsByColor.set(color, [option]); - } - } - - for (const [color, options] of optionsByColor) { - editor.setDecorations(getMemDecorationTypeForColor(color), options); - } - recordApplied(appliedMemDecorations, filePath, optionsByColor); - } - + leakLayer.paint(result.suspectedLeaks.map((leak) => ({ + file: leak.file, + line: leak.line, + color: leakColor(leak.confidence), + text: ` ${confidenceBadge(leak.confidence)} +${formatBytes(Math.abs(leak.sizeGrowth))} leak (${leak.countGrowth} objects)`, + }))); Logger.info(`Leak decorations applied: ${result.suspectedLeaks.length} suspected leaks`); } /** Clear all memory decorations from visible editors. */ export function clearMemoryDecorations(): void { - for (const decorationType of memDecorationTypes.values()) { - for (const editor of vscode.window.visibleTextEditors) { - editor.setDecorations(decorationType, []); - } - } - appliedMemDecorations = []; + allocationLayer.clear(); + leakLayer.clear(); } /** Dispose all memory decoration types. */ export function disposeMemoryDecorations(): void { - for (const decorationType of memDecorationTypes.values()) { - decorationType.dispose(); - } - memDecorationTypes.clear(); - appliedMemDecorations = []; + allocationLayer.dispose(); + leakLayer.dispose(); } From de31c4e60ede67636b087691eb64135e0973fd9d Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:53:15 +1100 Subject: [PATCH 4/5] Report a truncated retention graph when instances past the targets' share are left out refs keeps at most a quarter of the node budget as targets ([PROF-DOTNET-HEAP-MARK]) and dropped every other instance, yet said truncated only when the node cap was hit. Two Orders out of 2,000 filled 4 of 8 nodes and the graph claimed to be complete. --- dotnet/LspProfiler.DotNet.Engine/RetentionGraph.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dotnet/LspProfiler.DotNet.Engine/RetentionGraph.cs b/dotnet/LspProfiler.DotNet.Engine/RetentionGraph.cs index 002179a..393f019 100644 --- a/dotnet/LspProfiler.DotNet.Engine/RetentionGraph.cs +++ b/dotnet/LspProfiler.DotNet.Engine/RetentionGraph.cs @@ -16,15 +16,17 @@ public static Refs Build(LiveHeap live, string typeName, int maxDepth, int maxNo var targets = live.Parents.Keys .Where(node => string.Equals(live.Graph.TypeOf(node).Name, typeName, StringComparison.Ordinal)) .OrderByDescending(live.Graph.SizeOf) - .Take(Math.Max(1, maxNodes / TargetShare)); - foreach (var target in targets) + .ToList(); + var shown = Math.Max(1, maxNodes / TargetShare); + foreach (var target in targets.Take(shown)) { if (!builder.AddTarget(target)) { break; } } - return builder.Finish(); + // Instances past the targets' share are left out: the budget cut the walk short. + return builder.Finish(omittedTargets: targets.Count > shown); } private sealed class Builder(LiveHeap live, int maxDepth, int maxNodes) @@ -54,7 +56,8 @@ public bool AddTarget(int target) return true; } - public Refs Finish() => new("refs", [.. _nodes.Values], _edges, _truncated); + public Refs Finish(bool omittedTargets) => + new("refs", [.. _nodes.Values], _edges, _truncated || omittedTargets); private bool Connect(int holder, int held, int depth) { From d60fd858c0f33ef2e505ddb5913da4459017df85 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:57:18 +1100 Subject: [PATCH 5/5] Count a frame once per tick, resolve the named runtime first, keep a memory step's classified failure - Aggregation deduplicated lines and functions per thread stack, so a frame every thread shares (the runtime's thread entry point) counted once per thread and its samples outnumbered the ticks. Lines, functions and self time now count once per tick across every thread's stack, and shares are of the ticks that kept a stack (user_samples) ([PROF-AGGREGATION-LOGIC]). - cpu.start checked for an existing session before resolving the runtime, so naming a runtime that cannot profile a busy PID answered ALREADY_PROFILING instead of UNSUPPORTED_RUNTIME ([PROF-CPU-START] step 1). - A memory step folded every engine failure into OTHER, so a snapshot of a program that had exited lost the engine's PROCESS_NOT_FOUND. Classified attach failures now keep their code ([PROF-ERRORS]). - aggregate.rs's tests move to aggregate/tests.rs to keep the file under 500 lines. --- crates/lsp-profiler-dotnet/src/cpu.rs | 3 +- crates/lsp-profiler-dotnet/src/memory.rs | 6 +- .../lsp-profiler-dotnet/tests/dotnet_e2e.rs | 52 ++++ crates/lsp-profiler/src/aggregate.rs | 255 ++--------------- crates/lsp-profiler/src/aggregate/tests.rs | 263 ++++++++++++++++++ crates/lsp-profiler/src/error.rs | 29 +- crates/lsp-profiler/src/memory/mod.rs | 10 +- crates/lsp-profiler/src/service.rs | 2 +- docs/models/profiler.td | 2 +- docs/specs/LSP-PROFILER-SPEC.md | 2 +- 10 files changed, 389 insertions(+), 235 deletions(-) create mode 100644 crates/lsp-profiler/src/aggregate/tests.rs diff --git a/crates/lsp-profiler-dotnet/src/cpu.rs b/crates/lsp-profiler-dotnet/src/cpu.rs index 9c616c2..979ce0e 100644 --- a/crates/lsp-profiler-dotnet/src/cpu.rs +++ b/crates/lsp-profiler-dotnet/src/cpu.rs @@ -214,7 +214,8 @@ fn sampler_error(error: EngineError, pid: u32) -> SamplerError { } } -fn attach_kind(kind: ErrorKind) -> Option { +/// The attach failure an engine error kind classifies, if any. +pub(crate) fn attach_kind(kind: ErrorKind) -> Option { match kind { ErrorKind::ProcessNotFound => Some(AttachErrorKind::ProcessNotFound), ErrorKind::UnsupportedRuntime => Some(AttachErrorKind::UnsupportedRuntime), diff --git a/crates/lsp-profiler-dotnet/src/memory.rs b/crates/lsp-profiler-dotnet/src/memory.rs index 3c176c9..610400a 100644 --- a/crates/lsp-profiler-dotnet/src/memory.rs +++ b/crates/lsp-profiler-dotnet/src/memory.rs @@ -18,6 +18,7 @@ use lsp_profiler::wire::{ use tokio::sync::Mutex; use tracing::{debug, warn}; +use crate::cpu::attach_kind; use crate::engine::{Engine, EngineError}; use crate::protocol::{EngineMessage, ObjectRecord, RefEdgeRecord, RefNodeRecord, TypeRow}; @@ -348,7 +349,10 @@ fn object(item: ObjectRecord) -> ObjectSample { fn memory_error(error: EngineError) -> MemoryError { match error { EngineError::Missing(message) => MemoryError::ToolMissing(message), - EngineError::Failed { message, .. } => MemoryError::Backend(message), + EngineError::Failed { kind, message } => match attach_kind(kind) { + Some(kind) => MemoryError::Attach { kind, message }, + None => MemoryError::Backend(message), + }, other => { warn!(%other, "the .NET profiling engine failed"); MemoryError::Backend(other.to_string()) diff --git a/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs b/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs index 736de88..81f1b22 100644 --- a/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs +++ b/crates/lsp-profiler-dotnet/tests/dotnet_e2e.rs @@ -261,6 +261,58 @@ async fn a_live_process_no_runtime_recognises_is_unsupported_and_never_killed() service.shutdown().await; } +/// `cpu.start` resolves the runtime before anything else ([PROF-CPU-START] +/// step 1): naming a runtime that cannot profile the PID is +/// `UNSUPPORTED_RUNTIME` even while the PID has a CPU session, and only a +/// start that could attach is `ALREADY_PROFILING`. +#[tokio::test(flavor = "multi_thread")] +async fn a_named_runtime_is_resolved_before_the_pid_is_found_busy() { + let fixture = Fixture::start(FIXTURE); + let dir = tempfile::tempdir().expect("tempdir"); + let service = service(dir.path().to_path_buf()); + let started = call(&service, methods::CPU_START, json!({ "pid": fixture.pid })).await; + let named = json!({ "pid": fixture.pid, "runtime": "python" }); + assert_eq!( + error_code(&service, methods::CPU_START, named).await, + -32002 + ); + let again = json!({ "pid": fixture.pid }); + assert_eq!( + error_code(&service, methods::CPU_START, again).await, + -32004 + ); + let _ = call( + &service, + methods::CPU_STOP, + json!({ "session_id": started["session_id"] }), + ) + .await; + service.shutdown().await; +} + +/// A memory step on a program that has exited is `PROCESS_NOT_FOUND`, the +/// engine's classified cause, never a bare `OTHER` ([PROF-ERRORS]). +#[tokio::test(flavor = "multi_thread")] +async fn a_memory_step_on_an_exited_program_is_process_not_found() { + let mut fixture = Fixture::start(FIXTURE); + let dir = tempfile::tempdir().expect("tempdir"); + let service = service(dir.path().to_path_buf()); + let started = call( + &service, + methods::MEMORY_START, + json!({ "pid": fixture.pid }), + ) + .await; + let session = json!({ "memory_session_id": started["memory_session_id"] }); + let _ = fixture.child.kill(); + let _ = fixture.child.wait(); + assert_eq!( + error_code(&service, methods::MEMORY_SNAPSHOT, session).await, + -32001 + ); + service.shutdown().await; +} + /// A target that exits mid-session keeps the samples taken while it lived: the /// recorded trace is still symbolized and reported on stop ([PROF-CPU-STOP]). #[tokio::test(flavor = "multi_thread")] diff --git a/crates/lsp-profiler/src/aggregate.rs b/crates/lsp-profiler/src/aggregate.rs index 39accc2..2d9cabf 100644 --- a/crates/lsp-profiler/src/aggregate.rs +++ b/crates/lsp-profiler/src/aggregate.rs @@ -21,6 +21,8 @@ pub struct ProfileData { pub function_stats: HashMap, /// Sampling ticks ingested. pub total_samples: u64, + /// Ticks in which at least one thread's stack was kept. + pub busy_ticks: u64, /// Kept thread samples per thread. pub thread_samples: HashMap, /// Frame deduplication index for exports. @@ -101,12 +103,14 @@ impl Default for HotspotConfig { } } -/// Per-sample dedup sets: an inclusive counter credits each distinct line or -/// function once per sample, so recursion can never push a share above 100%. +/// Per-tick dedup sets: a counter credits each distinct line or function once +/// per tick, across every thread's stack, so neither recursion nor a frame +/// every thread shares can push a share above 100%. #[derive(Default)] struct SampleSeen { lines: HashSet<(String, i64)>, functions: HashSet, + leaves: HashSet, } impl ProfileData { @@ -115,7 +119,8 @@ impl ProfileData { /// Idle threads are skipped unless `include_idle`; scaffolding frames are /// stripped first and a thread left with nothing is dropped as pure /// machinery ([PROF-AGGREGATION-SCAFFOLD]). The tick counts once in - /// `total_samples` whatever its thread count. + /// `total_samples` whatever its thread count, and once in `busy_ticks` + /// when any thread's stack is kept. pub fn ingest( &mut self, batch: &SampleBatch, @@ -124,6 +129,8 @@ impl ProfileData { filter: &dyn FrameFilter, ) { let weight = batch.weight_secs.unwrap_or(default_weight); + let mut seen = SampleSeen::default(); + let mut busy = false; for sample in batch.samples.iter().filter(|s| s.active || include_idle) { let kept: Vec<&StackFrame> = sample .frames @@ -131,14 +138,22 @@ impl ProfileData { .filter(|frame| !filter.is_scaffolding(frame)) .collect(); if !kept.is_empty() { - self.ingest_thread(sample, &kept, weight); + self.ingest_thread(sample, &kept, weight, &mut seen); + busy = true; } } + self.busy_ticks += u64::from(busy); self.total_samples += 1; } /// Record one kept thread stack (frames leaf first). - fn ingest_thread(&mut self, sample: &ThreadSample, kept: &[&StackFrame], weight: f64) { + fn ingest_thread( + &mut self, + sample: &ThreadSample, + kept: &[&StackFrame], + weight: f64, + seen: &mut SampleSeen, + ) { let thread_id = sample.thread_id; if let Some(name) = &sample.thread_name { let _ = self @@ -147,11 +162,10 @@ impl ProfileData { .or_insert_with(|| name.clone()); } *self.thread_samples.entry(thread_id).or_insert(0) += 1; - let mut seen = SampleSeen::default(); let mut stack: Vec = kept .iter() .enumerate() - .map(|(depth, frame)| self.ingest_frame(frame, depth == 0, &mut seen)) + .map(|(depth, frame)| self.ingest_frame(frame, depth == 0, seen)) .collect(); // Samplers report leaf first; exports want root first. stack.reverse(); @@ -179,6 +193,7 @@ impl ProfileData { name: frame.name.clone(), }; let newly_seen = seen.functions.insert(key.clone()); + let newly_leaf = is_leaf && seen.leaves.insert(key.clone()); let stats = self .function_stats .entry(key) @@ -190,7 +205,7 @@ impl ProfileData { self_samples: 0, }); stats.total_samples += u64::from(newly_seen); - stats.self_samples += u64::from(is_leaf); + stats.self_samples += u64::from(newly_leaf); self.frame_slot(FrameKey::of(frame)) } @@ -205,11 +220,11 @@ impl ProfileData { index } - /// Kept thread samples — the single denominator of every percentage. - /// Equals the sum of self samples, since each kept stack has one leaf. + /// Ticks that kept a thread stack — the single denominator of every + /// percentage. Every count is per tick, so no share exceeds 100%. #[must_use] pub fn sample_count(&self) -> u64 { - self.thread_samples.values().sum() + self.busy_ticks } /// Lines at or above the line threshold, hottest first, capped per file. @@ -290,220 +305,4 @@ pub fn saturating_i64(value: u64) -> i64 { } #[cfg(test)] -mod tests { - use super::*; - use crate::runtime::KeepAllFrames; - - fn frame(name: &str, file: &str, line: i64) -> StackFrame { - StackFrame { - name: name.to_owned(), - file: Some(file.to_owned()), - line: Some(line), - module: None, - } - } - - fn tick(thread_id: i64, active: bool, frames: Vec) -> SampleBatch { - SampleBatch::new(vec![ThreadSample { - thread_id, - thread_name: Some(format!("Thread-{thread_id}")), - active, - frames, - }]) - } - - fn ingest(data: &mut ProfileData, batch: &SampleBatch) { - data.ingest(batch, 0.01, false, &KeepAllFrames); - } - - fn stats<'a>(data: &'a ProfileData, file: &str, name: &str) -> Option<&'a FunctionStats> { - data.function_stats.get(&FunctionKey { - file: Some(file.to_owned()), - name: name.to_owned(), - }) - } - - // [PROF-AGGREGATION-LOGIC] Leaf gets self samples; every frame gets an - // inclusive count and a line hit. - #[test] - fn ingest_single_trace_counts_correctly() { - let mut data = ProfileData::default(); - let stack = vec![ - frame("leaf_fn", "src/a.py", 10), - frame("caller_fn", "src/a.py", 5), - frame("main", "main.py", 1), - ]; - ingest(&mut data, &tick(1, true, stack)); - assert_eq!(data.total_samples, 1); - assert_eq!( - data.line_hits.get("src/a.py").and_then(|m| m.get(&10)), - Some(&1) - ); - let leaf = stats(&data, "src/a.py", "leaf_fn").map(|s| (s.total_samples, s.self_samples)); - assert_eq!(leaf, Some((1, 1))); - let caller = - stats(&data, "src/a.py", "caller_fn").map(|s| (s.total_samples, s.self_samples)); - assert_eq!(caller, Some((1, 0))); - let root_first: Vec<&str> = data - .thread_stacks - .get(&1) - .and_then(|stacks| stacks.first()) - .map(|stack| { - stack - .iter() - .filter_map(|i| data.frames.get(*i)) - .map(|f| f.name.as_str()) - .collect() - }) - .unwrap_or_default(); - assert_eq!(root_first, vec!["main", "caller_fn", "leaf_fn"]); - } - - // [PROF-AGGREGATION-LOGIC] Idle threads are skipped unless included. - #[test] - fn idle_traces_skipped_when_not_included() { - let mut data = ProfileData::default(); - ingest( - &mut data, - &tick(1, false, vec![frame("idle_fn", "src/a.py", 10)]), - ); - assert_eq!(data.total_samples, 1); - assert!(data.line_hits.is_empty()); - data.ingest( - &tick(1, false, vec![frame("idle_fn", "src/a.py", 10)]), - 0.01, - true, - &KeepAllFrames, - ); - assert_eq!( - data.line_hits.get("src/a.py").and_then(|m| m.get(&10)), - Some(&1) - ); - } - - // [PROF-AGGREGATION-LOGIC] Recursion counts once per sample, so no share - // can exceed 100%. - #[test] - fn recursive_stack_counts_inclusive_totals_once_per_sample() { - let mut data = ProfileData::default(); - let stack = vec![ - frame("build_topo", "src/a.py", 98), - frame("build_topo", "src/a.py", 101), - frame("build_topo", "src/a.py", 101), - frame("main", "main.py", 1), - ]; - ingest(&mut data, &tick(1, true, stack)); - let topo = - stats(&data, "src/a.py", "build_topo").map(|s| (s.total_samples, s.self_samples)); - assert_eq!(topo, Some((1, 1)), "one sample credits one inclusive count"); - assert_eq!( - data.line_hits.get("src/a.py").and_then(|m| m.get(&101)), - Some(&1) - ); - let config = HotspotConfig { - function_threshold_pct: 0.0, - ..HotspotConfig::default() - }; - let hot = data.hot_functions(&config); - let topo_pct = hot - .iter() - .find(|f| f.name == "build_topo") - .map(|f| f.percentage); - assert_eq!(topo_pct, Some(100.0)); - let line_pct = data - .hot_lines(&HotspotConfig::default()) - .iter() - .find(|l| l.line == 101) - .map(|l| l.percentage); - assert_eq!(line_pct, Some(100.0)); - } - - // [PROF-AGGREGATION-THRESHOLD] Lines below the threshold are dropped. - #[test] - fn hot_lines_filters_by_threshold() { - let mut data = ProfileData::default(); - for _ in 0..50 { - ingest( - &mut data, - &tick(1, true, vec![frame("fn_a", "src/a.py", 10)]), - ); - } - ingest( - &mut data, - &tick(1, true, vec![frame("fn_b", "src/a.py", 20)]), - ); - let config = HotspotConfig { - line_threshold_pct: 5.0, - ..HotspotConfig::default() - }; - let lines: Vec = data.hot_lines(&config).iter().map(|h| h.line).collect(); - assert_eq!(lines, vec![10]); - } - - // [PROF-AGGREGATION-STRUCTS] Frames without a source location (a runtime - // that reports only names) still yield hot functions, never hot lines. - #[test] - fn name_only_frames_produce_functions_but_no_lines() { - let mut data = ProfileData::default(); - let nameless = StackFrame { - name: "Ns.Type.Work()".to_owned(), - file: None, - line: None, - module: Some("App".to_owned()), - }; - ingest(&mut data, &tick(4, true, vec![nameless])); - assert!(data.line_hits.is_empty()); - let hot = data.hot_functions(&HotspotConfig::default()); - assert_eq!( - hot.first() - .map(|f| (f.name.as_str(), f.file.is_none(), f.percentage)), - Some(("Ns.Type.Work()", true, 100.0)) - ); - } - - // [PROF-AGGREGATION-SCAFFOLD] A runtime filter strips plumbing frames, and - // a thread left with nothing is dropped entirely. - #[test] - fn filter_strips_scaffolding_and_drops_machinery_threads() { - #[derive(Debug)] - struct DropLauncher; - impl FrameFilter for DropLauncher { - fn is_scaffolding(&self, frame: &StackFrame) -> bool { - frame.name == "launcher" - } - } - let mut data = ProfileData::default(); - let batch = SampleBatch::new(vec![ - ThreadSample { - thread_id: 1, - thread_name: None, - active: true, - frames: vec![frame("work", "a.py", 3), frame("launcher", "l.py", 1)], - }, - ThreadSample { - thread_id: 2, - thread_name: None, - active: true, - frames: vec![frame("launcher", "l.py", 1)], - }, - ]); - data.ingest(&batch, 0.01, false, &DropLauncher); - assert_eq!( - data.sample_count(), - 1, - "the machinery-only thread is dropped" - ); - assert!(data.frames.iter().all(|f| f.name != "launcher")); - assert!(!data.thread_samples.contains_key(&2)); - } - - // [PROF-AGGREGATION-LOGIC] A batch weight overrides the session default. - #[test] - fn batch_weight_overrides_default() { - let mut data = ProfileData::default(); - let mut batch = tick(1, true, vec![frame("f", "a.py", 1)]); - batch.weight_secs = Some(0.25); - ingest(&mut data, &batch); - assert_eq!(data.thread_weights.get(&1).cloned(), Some(vec![0.25])); - } -} +mod tests; diff --git a/crates/lsp-profiler/src/aggregate/tests.rs b/crates/lsp-profiler/src/aggregate/tests.rs new file mode 100644 index 0000000..9e307fa --- /dev/null +++ b/crates/lsp-profiler/src/aggregate/tests.rs @@ -0,0 +1,263 @@ +use super::*; +use crate::runtime::KeepAllFrames; + +fn frame(name: &str, file: &str, line: i64) -> StackFrame { + StackFrame { + name: name.to_owned(), + file: Some(file.to_owned()), + line: Some(line), + module: None, + } +} + +fn tick(thread_id: i64, active: bool, frames: Vec) -> SampleBatch { + SampleBatch::new(vec![ThreadSample { + thread_id, + thread_name: Some(format!("Thread-{thread_id}")), + active, + frames, + }]) +} + +fn ingest(data: &mut ProfileData, batch: &SampleBatch) { + data.ingest(batch, 0.01, false, &KeepAllFrames); +} + +fn stats<'a>(data: &'a ProfileData, file: &str, name: &str) -> Option<&'a FunctionStats> { + data.function_stats.get(&FunctionKey { + file: Some(file.to_owned()), + name: name.to_owned(), + }) +} + +// [PROF-AGGREGATION-LOGIC] Leaf gets self samples; every frame gets an +// inclusive count and a line hit. +#[test] +fn ingest_single_trace_counts_correctly() { + let mut data = ProfileData::default(); + let stack = vec![ + frame("leaf_fn", "src/a.py", 10), + frame("caller_fn", "src/a.py", 5), + frame("main", "main.py", 1), + ]; + ingest(&mut data, &tick(1, true, stack)); + assert_eq!(data.total_samples, 1); + assert_eq!( + data.line_hits.get("src/a.py").and_then(|m| m.get(&10)), + Some(&1) + ); + let leaf = stats(&data, "src/a.py", "leaf_fn").map(|s| (s.total_samples, s.self_samples)); + assert_eq!(leaf, Some((1, 1))); + let caller = stats(&data, "src/a.py", "caller_fn").map(|s| (s.total_samples, s.self_samples)); + assert_eq!(caller, Some((1, 0))); + let root_first: Vec<&str> = data + .thread_stacks + .get(&1) + .and_then(|stacks| stacks.first()) + .map(|stack| { + stack + .iter() + .filter_map(|i| data.frames.get(*i)) + .map(|f| f.name.as_str()) + .collect() + }) + .unwrap_or_default(); + assert_eq!(root_first, vec!["main", "caller_fn", "leaf_fn"]); +} + +// [PROF-AGGREGATION-LOGIC] Idle threads are skipped unless included. +#[test] +fn idle_traces_skipped_when_not_included() { + let mut data = ProfileData::default(); + ingest( + &mut data, + &tick(1, false, vec![frame("idle_fn", "src/a.py", 10)]), + ); + assert_eq!(data.total_samples, 1); + assert!(data.line_hits.is_empty()); + data.ingest( + &tick(1, false, vec![frame("idle_fn", "src/a.py", 10)]), + 0.01, + true, + &KeepAllFrames, + ); + assert_eq!( + data.line_hits.get("src/a.py").and_then(|m| m.get(&10)), + Some(&1) + ); +} + +// [PROF-AGGREGATION-LOGIC] Recursion counts once per sample, so no share +// can exceed 100%. +#[test] +fn recursive_stack_counts_inclusive_totals_once_per_sample() { + let mut data = ProfileData::default(); + let stack = vec![ + frame("build_topo", "src/a.py", 98), + frame("build_topo", "src/a.py", 101), + frame("build_topo", "src/a.py", 101), + frame("main", "main.py", 1), + ]; + ingest(&mut data, &tick(1, true, stack)); + let topo = stats(&data, "src/a.py", "build_topo").map(|s| (s.total_samples, s.self_samples)); + assert_eq!(topo, Some((1, 1)), "one sample credits one inclusive count"); + assert_eq!( + data.line_hits.get("src/a.py").and_then(|m| m.get(&101)), + Some(&1) + ); + let config = HotspotConfig { + function_threshold_pct: 0.0, + ..HotspotConfig::default() + }; + let hot = data.hot_functions(&config); + let topo_pct = hot + .iter() + .find(|f| f.name == "build_topo") + .map(|f| f.percentage); + assert_eq!(topo_pct, Some(100.0)); + let line_pct = data + .hot_lines(&HotspotConfig::default()) + .iter() + .find(|l| l.line == 101) + .map(|l| l.percentage); + assert_eq!(line_pct, Some(100.0)); +} + +// [PROF-AGGREGATION-THRESHOLD] Lines below the threshold are dropped. +#[test] +fn hot_lines_filters_by_threshold() { + let mut data = ProfileData::default(); + for _ in 0..50 { + ingest( + &mut data, + &tick(1, true, vec![frame("fn_a", "src/a.py", 10)]), + ); + } + ingest( + &mut data, + &tick(1, true, vec![frame("fn_b", "src/a.py", 20)]), + ); + let config = HotspotConfig { + line_threshold_pct: 5.0, + ..HotspotConfig::default() + }; + let lines: Vec = data.hot_lines(&config).iter().map(|h| h.line).collect(); + assert_eq!(lines, vec![10]); +} + +// [PROF-AGGREGATION-STRUCTS] Frames without a source location (a runtime +// that reports only names) still yield hot functions, never hot lines. +#[test] +fn name_only_frames_produce_functions_but_no_lines() { + let mut data = ProfileData::default(); + let nameless = StackFrame { + name: "Ns.Type.Work()".to_owned(), + file: None, + line: None, + module: Some("App".to_owned()), + }; + ingest(&mut data, &tick(4, true, vec![nameless])); + assert!(data.line_hits.is_empty()); + let hot = data.hot_functions(&HotspotConfig::default()); + assert_eq!( + hot.first() + .map(|f| (f.name.as_str(), f.file.is_none(), f.percentage)), + Some(("Ns.Type.Work()", true, 100.0)) + ); +} + +// [PROF-AGGREGATION-SCAFFOLD] A runtime filter strips plumbing frames, and +// a thread left with nothing is dropped entirely. +#[test] +fn filter_strips_scaffolding_and_drops_machinery_threads() { + #[derive(Debug)] + struct DropLauncher; + impl FrameFilter for DropLauncher { + fn is_scaffolding(&self, frame: &StackFrame) -> bool { + frame.name == "launcher" + } + } + let mut data = ProfileData::default(); + let batch = SampleBatch::new(vec![ + ThreadSample { + thread_id: 1, + thread_name: None, + active: true, + frames: vec![frame("work", "a.py", 3), frame("launcher", "l.py", 1)], + }, + ThreadSample { + thread_id: 2, + thread_name: None, + active: true, + frames: vec![frame("launcher", "l.py", 1)], + }, + ]); + data.ingest(&batch, 0.01, false, &DropLauncher); + assert_eq!( + data.sample_count(), + 1, + "the machinery-only thread is dropped" + ); + assert!(data.frames.iter().all(|f| f.name != "launcher")); + assert!(!data.thread_samples.contains_key(&2)); +} + +// [PROF-AGGREGATION-LOGIC] A batch weight overrides the session default. +#[test] +fn batch_weight_overrides_default() { + let mut data = ProfileData::default(); + let mut batch = tick(1, true, vec![frame("f", "a.py", 1)]); + batch.weight_secs = Some(0.25); + ingest(&mut data, &batch); + assert_eq!(data.thread_weights.get(&1).cloned(), Some(vec![0.25])); +} + +// [PROF-AGGREGATION-LOGIC] A function on several threads' stacks in one tick +// (every managed thread starts in the runtime's thread entry point) counts once +// for that tick, inclusive and self, so its samples never outnumber the ticks. +#[test] +fn threads_sharing_a_function_in_one_tick_count_it_once() { + let mut data = ProfileData::default(); + let thread = |thread_id, leaf| ThreadSample { + thread_id, + thread_name: None, + active: true, + frames: vec![frame(leaf, "src/a.py", 7), frame("start", "src/a.py", 1)], + }; + ingest( + &mut data, + &SampleBatch::new(vec![thread(1, "work"), thread(2, "work")]), + ); + ingest( + &mut data, + &SampleBatch::new(vec![thread(1, "work"), thread(2, "other")]), + ); + assert_eq!(data.total_samples, 2); + let counts = |name| stats(&data, "src/a.py", name).map(|s| (s.total_samples, s.self_samples)); + assert_eq!( + counts("start"), + Some((2, 0)), + "one count per tick, not per thread" + ); + assert_eq!( + counts("work"), + Some((2, 2)), + "self counts once per tick too" + ); + assert_eq!(counts("other"), Some((1, 1))); + assert_eq!( + data.line_hits + .get("src/a.py") + .map(|lines| (lines.get(&1), lines.get(&7))), + Some((Some(&2), Some(&2))), + "a line counts once per tick" + ); + let config = HotspotConfig { + function_threshold_pct: 0.0, + ..HotspotConfig::default() + }; + assert!(data + .hot_functions(&config) + .iter() + .all(|f| f.samples <= 2 && f.percentage <= 100.0 && f.self_percentage <= f.percentage)); +} diff --git a/crates/lsp-profiler/src/error.rs b/crates/lsp-profiler/src/error.rs index c2ed13a..d158ece 100644 --- a/crates/lsp-profiler/src/error.rs +++ b/crates/lsp-profiler/src/error.rs @@ -2,7 +2,7 @@ use crate::memory::MemoryError; use crate::sampler::SamplerError; -use crate::wire::error_codes; +use crate::wire::{error_codes, AttachErrorKind}; /// Any failure of a profiler request. #[derive(Debug, Clone, PartialEq, thiserror::Error)] @@ -49,12 +49,23 @@ impl ProfilerError { Self::Sampler(SamplerError::AttachFailed(_)) => error_codes::ATTACH_FAILED, Self::Sampler(SamplerError::ToolMissing(_)) | Self::Memory(MemoryError::ToolMissing(_)) => error_codes::TOOL_MISSING, + Self::Memory(MemoryError::Attach { kind, .. }) => attach_code(kind), Self::InvalidParams(_) => error_codes::INVALID_PARAMS, Self::Memory(_) | Self::SessionNotFound(_) | Self::Other(_) => error_codes::OTHER, } } } +/// The code of a backend's classified attach failure. +fn attach_code(kind: &AttachErrorKind) -> i64 { + match kind { + AttachErrorKind::ProcessNotFound => error_codes::PROCESS_NOT_FOUND, + AttachErrorKind::UnsupportedRuntime => error_codes::UNSUPPORTED_RUNTIME, + AttachErrorKind::PermissionDenied => error_codes::PERMISSION_DENIED, + AttachErrorKind::AttachFailed => error_codes::ATTACH_FAILED, + } +} + #[cfg(test)] mod tests { use super::*; @@ -95,6 +106,22 @@ mod tests { (ProfilerError::InvalidParams("bad".to_owned()), -32602), (ProfilerError::SessionNotFound("s".to_owned()), -32000), (MemoryError::UnknownSession("m".to_owned()).into(), -32000), + ( + MemoryError::Attach { + kind: AttachErrorKind::ProcessNotFound, + message: "gone".to_owned(), + } + .into(), + -32001, + ), + ( + MemoryError::Attach { + kind: AttachErrorKind::PermissionDenied, + message: "grant".to_owned(), + } + .into(), + -32003, + ), ]; for (error, code) in cases { assert_eq!(error.code(), code, "{error}"); diff --git a/crates/lsp-profiler/src/memory/mod.rs b/crates/lsp-profiler/src/memory/mod.rs index 2e8d8d8..a82037c 100644 --- a/crates/lsp-profiler/src/memory/mod.rs +++ b/crates/lsp-profiler/src/memory/mod.rs @@ -15,7 +15,7 @@ pub mod leaks; pub mod session; pub mod timeline; -use crate::wire::{GcResult, HeapDiff, HeapSnapshot, ObjectList, RefGraph}; +use crate::wire::{AttachErrorKind, GcResult, HeapDiff, HeapSnapshot, ObjectList, RefGraph}; /// What a backend wants done for one memory step. #[derive(Debug, Clone, PartialEq)] @@ -64,6 +64,14 @@ pub enum MemoryError { /// A required external tool is missing; the message says how to install it. #[error("{0}")] ToolMissing(String), + /// The backend could not reach the process; `kind` classifies why. + #[error("{message}")] + Attach { + /// Classified cause. + kind: AttachErrorKind, + /// Backend detail. + message: String, + }, /// The backend failed. #[error("{0}")] Backend(String), diff --git a/crates/lsp-profiler/src/service.rs b/crates/lsp-profiler/src/service.rs index 274e016..f66b32f 100644 --- a/crates/lsp-profiler/src/service.rs +++ b/crates/lsp-profiler/src/service.rs @@ -253,8 +253,8 @@ impl ProfilerService { }, ) .map_err(ProfilerError::InvalidParams)?; - self.inner.cpu.ensure_free(pid).await?; let runtime = self.cpu_runtime(request.runtime.as_deref(), pid).await?; + self.inner.cpu.ensure_free(pid).await?; let sampler = self.attach(runtime.as_ref(), &config).await?; let started = self .inner diff --git a/docs/models/profiler.td b/docs/models/profiler.td index 249fb14..964cabf 100644 --- a/docs/models/profiler.td +++ b/docs/models/profiler.td @@ -364,7 +364,7 @@ type ProfileReport { duration_secs: Float # Sampling ticks taken. total_samples: Int - # Thread samples that survived runtime filtering (user code). + # Ticks in which a thread sample survived runtime filtering (user code): every share's denominator. user_samples: Int # Samples per second. sample_rate: Int diff --git a/docs/specs/LSP-PROFILER-SPEC.md b/docs/specs/LSP-PROFILER-SPEC.md index 2b7c007..1e5e09e 100644 --- a/docs/specs/LSP-PROFILER-SPEC.md +++ b/docs/specs/LSP-PROFILER-SPEC.md @@ -315,7 +315,7 @@ It also holds the total and per-thread tick counts, deduplicated frames, thread ### [PROF-AGGREGATION-LOGIC] Logic -Each tick adds one to `total_samples`. A retained thread stack increments each distinct line and function at most once per tick, so recursion never pushes a share past 100%. The leaf increments self time. Idle threads count only when `include_idle` is set. Weights come from `weight_secs`, otherwise from `1 / sample_rate`. +Each tick adds one to `total_samples`. A retained thread stack increments each distinct line and function at most once per tick, across every thread's stack, so neither recursion nor a frame every thread shares (a runtime's thread entry point) pushes a share past 100%. The leaf increments self time, also once per tick. Shares are of the ticks that retained at least one stack, reported as `user_samples`. Idle threads count only when `include_idle` is set. Weights come from `weight_secs`, otherwise from `1 / sample_rate`. ### [PROF-AGGREGATION-SCAFFOLD] Runtime filtering