Add .NET performance analysis skill and optimizing agent - #6
Conversation
fffd2de to
a354975
Compare
|
Toub is reviewing results. |
|
found a critical bug in the agent. |
|
Prompt:
My test repo is Humanizr/Humanizer. |
|
Updated https://gist.github.com/artl93/5efc3dd3a6386bc388890b1aa37898f4 that at least hits the top items of both skills (skilled / agent is "A", llm only ls "B", 2-pass combined is "C") |
98e8e07 to
08a0d2a
Compare
7a84b5c to
57adfa1
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive .NET performance analysis capability with two components: the analyzing-dotnet-performance skill for systematic anti-pattern detection and the optimizing-dotnet-performance agent for guided performance optimization. The skill implements a progressive disclosure pattern with 7 reference files covering ~50 performance patterns distilled from Stephen Toub's .NET performance blog series, organized by severity (Critical/Moderate/Info) with concrete code fixes.
Changes:
- Adds
analyzing-dotnet-performanceskill with progressive reference loading based on code signals - Adds
optimizing-dotnet-performanceagent implementing mandatory two-pass analysis (LLM intuition + systematic scanning) - Includes 4 test fixtures demonstrating critical anti-patterns and eval scenarios for validation
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/dotnet/agents/optimizing-dotnet-performance.agent.md | Defines agent with two-pass workflow: direct analysis followed by skill-based deep scan with deduplication |
| src/dotnet/skills/analyzing-dotnet-performance/SKILL.md | Main skill definition with progressive disclosure workflow, severity classification, and validation checklist |
| src/dotnet/skills/analyzing-dotnet-performance/references/critical-patterns.md | 17 critical patterns causing deadlocks, regressions, or excessive allocations with grep detection recipes |
| src/dotnet/skills/analyzing-dotnet-performance/references/async-patterns.md | 5 async/concurrency patterns including ValueTask usage, Channels, and false sharing mitigation |
| src/dotnet/skills/analyzing-dotnet-performance/references/collections-and-linq.md | 9 collection patterns covering FrozenDictionary, CollectionsMarshal, params span optimizations |
| src/dotnet/skills/analyzing-dotnet-performance/references/io-and-serialization.md | 6 I/O patterns for streaming, async FileStream, Memory overloads, and static readonly devirtualization |
| src/dotnet/skills/analyzing-dotnet-performance/references/memory-and-strings.md | 9 memory patterns including Span, stackalloc, UTF8 literals, and string allocation chains |
| src/dotnet/skills/analyzing-dotnet-performance/references/regex-patterns.md | 4 regex patterns for GeneratedRegex, NonBacktracking, EnumerateMatches, and span-based APIs |
| src/dotnet/skills/analyzing-dotnet-performance/references/structural-patterns.md | Absence-based detection for unsealed classes with scale-based severity thresholds |
| src/dotnet/tests/analyzing-dotnet-performance/fixtures/critical-antipatterns.cs | Test fixture demonstrating sync-over-async, ReDoS vulnerability, and string concatenation in loops |
| src/dotnet/tests/analyzing-dotnet-performance/fixtures/collections-and-allocations.cs | Test fixture showing LINQ on hot paths, ContainsKey+indexer double-lookup, and O(n²) duplicate detection |
| src/dotnet/tests/analyzing-dotnet-performance/fixtures/io-and-async-issues.cs | Test fixture demonstrating HttpClient per-instance, sequential async, and file reading anti-patterns |
| src/dotnet/tests/analyzing-dotnet-performance/fixtures/struct-missing-iequatable.cs | Test fixture showing struct without IEquatable causing boxing in comparisons |
| src/dotnet/tests/analyzing-dotnet-performance/eval.yaml | 4 test scenarios validating skill detection of critical patterns, collections issues, I/O problems, and struct performance concerns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeffschwMSFT
left a comment
There was a problem hiding this comment.
lgtm. I would like to see the output from the skill validator
Introduce a dotnet-performance-optimizer agent and the dotnet-performance-patterns skill with reference guides covering: - Async patterns - Collections and LINQ - Critical performance patterns - I/O and serialization - Memory and strings - Regex patterns
- Add two-pass analysis methodology (Pass 1: LLM-only, Pass 2: skill-based) - Consolidate skill references to match actual reference files - Remove references to non-existent skills (dotnet-jit-optimization, dotnet-async-patterns, dotnet-sync-primitives) - Update pattern count to 85+ to reflect current reference corpus
- Add structural-patterns.md reference file - Expand collections-and-linq, memory-and-strings, regex patterns references - Update critical-patterns with refined rules - Expand SKILL.md with additional workflow guidance
Apply recommendations from platform.claude.com best practices: - Remove sections Claude already knows (Core Expertise, Edge Cases, Key Principles, When to Ask, Tone) - these waste context tokens - Remove skill internal reference file listing (skill handles its own progressive disclosure) - Condense 138 lines to 55 lines (-60%) - Keep only what Claude needs: two-pass workflow, output format, skill reference, escalation guidance
Apply recommendations from platform.claude.com best practices: - Remove Purpose section (redundant with description frontmatter) - Remove When to Use (redundant with description triggers) - Consolidate signal table (11 rows → 5, merging same-reference rows) - Remove 50-line checklist example template (keep the rule, not the example) - Remove 25-line Recipe Completeness Verification (redundant with gate) - Remove 40-line Output Format example (Claude knows markdown) - Trim Validation from 8 items + gate to 5 essential items - Trim Common Pitfalls from 3-column to 2-column - Condense Steps 5+6 into single Step 5 - 311 lines → 134 lines (-57%)
Apply 'Claude is already smart' principle to reference files: - Remove 6 patterns Claude already knows (Interlocked, SemaphoreSlim dup, Array.Empty, Any() vs Count(), Enum.HasFlag, Random.Shared) - Remove IEquatable<T> duplicate (kept in structural-patterns.md) - Remove 'Why a Separate File' meta-commentary from structural-patterns.md - Remove all horizontal rules between patterns - Remove explanatory prose and obvious inline comments - Preserve all Detection sections, scan recipes, and Impact statements Total: 1,819 → 1,372 lines (-25%)
- dotnet-performance-patterns → analyzing-dotnet-performance - dotnet-performance-optimizer → optimizing-dotnet-performance - Update all cross-references in frontmatter and agent skill list
- Agent: Replace second-person trigger phrases and verbose examples with concise third-person description stating what it does and when to use it - Skill: Add specificity (85+ patterns, tiered severity) and clearer triggers - Both now follow: third person, specific key terms, what + when format
jeffschwMSFT
left a comment
There was a problem hiding this comment.
lgtm. I think once we have this interacting with other skills we may have more feedback. given the size we would want this to only apply when doing performance investigations. I wonder if the simd skill is better suited as part of this suite (we can explore once merged).
…ical-patterns.md Co-authored-by: Dan Moseley <danmose@microsoft.com>
danmoseley
left a comment
There was a problem hiding this comment.
I didn't read the whole thing line by line as it was already. I jumped around and also used AI. I think it's good
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Skill/reference changes: - Remove CollectionsMarshal.AsSpan section (too much churn per stephentoub) - Remove startup/initialization as examples of non-perf-sensitive code - Split Contains into own grep with false-positive note - Clarify 'exact counts' means grep match counts - GeneratedRegex: ALWAYS for static patterns, always beneficial - Rewrite regex engine section: static->GeneratedRegex, never remove NonBacktracking - Sealed class grep catches protected and file classes - StringComparison grep uses proper ERE, catches variable args Test fixture fixes: - Fix invalid regex character class in inflector-and-regex.cs - Fix Hungarian converter cache key (use original number) - Add truncation guard for truncationString >= length - Use explicit ordered array instead of dictionary keys for Farsi/Kurdish - Preserve negative sign in ToMetric null-symbol fallback - Remove stray double semicolon Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…date+cleanup step Restore content lost when orchestrator was rewritten as pure dispatcher, plus add preventative guidance addressing the three regressions found in run 25504147640 (resolution dropped from 39%->17% vs run 25497220848). orchestrator (code-testing-generator.agent.md): - Step 1: reference unit-test-generation.prompt.md and pre-pipeline git stash - Step 4: implementer dispatch prompt now embeds test-strength rubric (concrete-value assertions, N>=3 collection inputs, full-equality, no toBeTruthy-only) and file-location rules - Step 8: restore Coverage Gap Iteration (re-research/plan/implement narrowed scope when rubric items remain uncovered) - Step 9 NEW: Validate-and-cleanup dispatch to code-testing-builder which has terminal/edit access. Removes .testagent/, runs git diff --name-only, reverts SUSPICIOUS files (env/cfg/toml/source-outside-test). Addresses 6/12 manifest_fail cases caused by .testagent/ leaking into the patch. - Rules: add dotnet#10 (cleanup mandatory) and dotnet#11 (test strength belongs in implementer prompt, not tester post-hoc check) implementer (code-testing-implementer.agent.md): - Step 4a NEW: Test strength requirements with concrete bad/good examples, reserved-name keys for property iteratees, mutation-resistance check - Step 4b NEW: File-location and side-effect rules with naming decision tree - Rules: add dotnet#6 (test strength non-negotiable) and dotnet#7 (stay inside test dirs) planner (code-testing-planner.agent.md): - Step 4: Add allowed/forbidden file targets, naming decision tree, and scenario depth requirement (N>=3 elements with concrete expected values) Diagnosis from run 25504147640: - 10/41 patches contained .testagent/research.md or plan.md (no cleanup) - mutation_fail jumped from 18 to 24 (single-element collection inputs, type-only assertions allowed weaker tests through) - app-2ad2720a modified tests/test.env DB port (no rule against config edits) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ion assertions The 'Use proper collection assertions' scenario regressed hard in both eval arms (isolated -0.31, plugin -0.38) even though the skill activated and produced correct output. Root cause is a judge knowledge gap, not a skill bug: the pairwise judge (claude-opus-4.6) marked the skill's Assert.IsNotEmpty, Assert.HasCount, and Assert.ContainsSingle as 'fabricated / non-existent MSTest methods' and favored the baseline's Assert.AreEqual(3, x.Count) plus the genuinely-fake CollectionAssert.That.IsNotEmpty. Those three ARE real MSTest 3.8+ assertion APIs (Microsoft.Testing modern assertions; verified against Microsoft Learn and the MSTest 3.8 release notes), and are exactly what the writing-mstest-tests skill teaches — so the skill's output was more correct than the baseline it lost to. Clarify the scenario rubric to name these real MSTest 3.8+ methods as valid answers and add an explicit grading note that Assert.IsNotEmpty/IsEmpty/HasCount/ ContainsSingle/Contains/DoesNotContain must not be penalized as non-existent. This is the doc-recommended remedy for 'rubric penalizes valid alternatives' (eng/skill-validator/src/docs/InvestigatingResults.md, pattern #6); the skill content is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…r + CPM notes for Workforce
.NET Performance Analysis Skill & Agent
Adds the analyzing-dotnet-performance skill and optimizing-dotnet-performance agent — a distilled, actionable set of ~50 .NET performance patterns designed to detect and fix anti-patterns in real codebases.
Where this comes from
Compiled from Stephen Toub's .NET performance blog series spanning .NET Core 2.0 through .NET 10. The full reference was shrunk to fit a skill context window, organized into 7 topic-specific reference files loaded on-demand based on code signals.
What's in the skill
critical-patterns.mdasync-patterns.mdmemory-and-strings.mdcollections-and-linq.mdio-and-serialization.mdregex-patterns.mdstructural-patterns.mdAgent: Two-Pass Analysis
Key features
Real-world validation
Validated against Humanizer: +8.9% average speed, +5.1% memory reduction across 81 BenchmarkDotNet benchmarks with zero functional regressions. PR example.
Comparison across runs — Run "B" is Claude Opus with no skills, Run "H" is this agent + skill.
Files