Add optimizing-ef-core-queries skill (+8.7% eval, near-miss)#90
Conversation
Teaches EF Core query optimization: N+1 detection and fixes, AsNoTracking for read-only queries, compiled queries for hot paths, AsSplitQuery for cartesian explosion, and common query traps (client eval, ToList before Where). Eval results: +8.7% improvement (threshold: 10%, near-miss) Includes eval.yaml with N+1 fix scenario + negative test.
There was a problem hiding this comment.
Pull request overview
This PR adds a new skill for optimizing Entity Framework Core queries, focusing on fixing N+1 query problems, enabling appropriate tracking modes, and avoiding common performance pitfalls. The skill achieved an 8.7% improvement in evaluations, just 1.3% below the 10% threshold.
Changes:
- Added comprehensive EF Core query optimization skill documentation covering N+1 fixes, tracking modes, compiled queries, and common query traps
- Created evaluation tests with positive scenario (N+1 detection) and negative scenario (Dapper question rejection)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/dotnet/skills/optimizing-ef-core-queries/SKILL.md | Complete skill documentation with workflow steps, code examples, validation checklist, and common pitfalls |
| src/dotnet/tests/optimizing-ef-core-queries/eval.yaml | Two test scenarios validating skill activation for EF Core and non-activation for Dapper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| How do I fix the performance? I see hundreds of SQL queries in the logs. | ||
| assertions: | ||
| - type: "output_matches" | ||
| pattern: "(N\\+1|N\\+1|eager.load|Include|Select|projection)" |
There was a problem hiding this comment.
The regex pattern contains a duplicate: N\\+1|N\\+1 appears twice. The second occurrence should likely be N\+1 (without the double backslash) or should be removed entirely. This redundancy doesn't break functionality but makes the pattern harder to maintain and understand.
| pattern: "(N\\+1|N\\+1|eager.load|Include|Select|projection)" | |
| pattern: "(N\\+1|eager.load|Include|Select|projection)" |
| .Include(o => o.Items) | ||
| .ToListAsync(); | ||
|
|
||
| // Option 2: Split query (separate SQL, avoids cartesian explosion) |
There was a problem hiding this comment.
The term "cartesian" should be capitalized as "Cartesian" when referring to the Cartesian product, which is named after mathematician René Descartes. This is a proper noun and should follow standard capitalization conventions.
| // Option 2: Split query (separate SQL, avoids cartesian explosion) | |
| // Option 2: Split query (separate SQL, avoids Cartesian explosion) |
| - "Identified the N+1 query pattern as the root cause (lazy loading triggering per-row queries)" | ||
| - "Suggested using Include() for eager loading OR Select() projection to fix N+1" | ||
| - "Recommended AsNoTracking() since this is a read-only query" | ||
| - "Mentioned AsSplitQuery() as an option for multiple Includes to avoid cartesian explosion" |
There was a problem hiding this comment.
The term "cartesian" should be capitalized as "Cartesian" when referring to the Cartesian product, which is named after mathematician René Descartes. This is a proper noun and should follow standard capitalization conventions.
| - "Mentioned AsSplitQuery() as an option for multiple Includes to avoid cartesian explosion" | |
| - "Mentioned AsSplitQuery() as an option for multiple Includes to avoid Cartesian explosion" |
| .UseSqlServer(connectionString) | ||
| .LogTo(Console.WriteLine, LogLevel.Information) | ||
| .EnableSensitiveDataLogging() // shows parameter values (dev only!) | ||
| .EnableDetailedErrors(); |
There was a problem hiding this comment.
EnableSensitiveDataLogging() and EnableDetailedErrors() generally aren't useful for finding perf issues
|
|
||
| ### Step 2: Fix N+1 query patterns | ||
|
|
||
| **The #1 EF Core performance killer.** Happens when loading related entities in a loop. |
There was a problem hiding this comment.
This is a bit overstated, the example would only affect apps that use lazy-loading. It would be more helpful to look for lazy-loading in general, as it can cause perf problems outside of obvious loops.
There was a problem hiding this comment.
Yes. I would generally discourage lazy loading in general - not only can it cause N+1, but it also requires sync-only I/O, which is bad for scalability and other scenarios.
Though I do agree that lazy loading and N+1 does occur, it's likely one of the worst perf problems.
| ```csharp | ||
| // Define once as static | ||
| private static readonly Func<AppDbContext, int, Task<Order?>> GetOrderById = | ||
| EF.CompileAsyncQuery((AppDbContext db, int id) => |
There was a problem hiding this comment.
This will only make a measurable difference for complex queries
There was a problem hiding this comment.
Agreed - I would generally discourage using compiled queries unless there's a very confirmed, measured impact, and only as a sort of last-resort optimization. I'd leave this out of a generic "optimize EF" skill, even if it's in our docs.
| | `ToList()` before `Where()` | Loads entire table into memory | Filter first: `.Where().ToList()` | | ||
| | `Count()` to check existence | Scans all rows | Use `.Any()` instead | | ||
| | `.Select()` after `.Include()` | Include is ignored with projection | Remove Include, use Select only | | ||
| | `string.Contains()` in Where | May not translate, falls to client eval | Use `EF.Functions.Like()` for SQL LIKE | |
| |---------|----------| | ||
| | Lazy loading silently creating N+1 | Remove `Microsoft.EntityFrameworkCore.Proxies` or disable lazy loading | | ||
| | Global query filters forgotten in perf analysis | Check `HasQueryFilter` in model config; use `IgnoreQueryFilters()` if needed | | ||
| | `DbContext` kept alive too long | DbContext should be scoped (per-request); don't cache it | |
There was a problem hiding this comment.
Should also be combined with DbContext pooling
There was a problem hiding this comment.
Agreed - in general there should be a seperate section for DbContext pooling, that's likely to be a much more impactful optimization than compiled queries, plus it (usually) doesn't have any impact on the code beyond just turning it on.
Skill Validation Results — optimizing-ef-core-queries
Overall improvement: +12.9% (3 runs, not statistically significant) Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
There was a problem hiding this comment.
Thanks @mrsharm - FYI I'm just in the middle of working on an EF skill - let's see what we can merge together here.
In general I'm wondering... does it make sense to have multiple fine-grained skills (this one is for general optimizations only), as opposed to one EF skill with specific sub-sections under references? I'm not yet familiar enough with skills to understand how good agents are at loading references sub-section on-demand (as opposed to multiple top-level skills).
Related to the above, what I had in mind was an "always on" skill which the agent loads whenever it's working on applications that use EF. That means it's important to keep the skill short and focused, as it's expected to permanently occupy context window space for everyone working on EF code.
In constrast, we could separate skills (or sub-skills) for specific actions/gestures the user expresses, like "this fragment of EF thing is slow, optimize it".
Thoughts?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
let's merge this as we learn how to build skills and it can grow/change with more insight (including from the inflight skill) |
Summary
Adds the optimizing-ef-core-queries skill for fixing EF Core performance issues.
Eval Results
What the Skill Teaches
Files