Joe feedback b2, b3, b6, b7 from #215#235
Conversation
- b3: Change Serial Plan message to "MAXDOP is set to 1 using a query hint" when user explicitly wrote MAXDOP 1 in the query - b6: Suppress Rule 6 (Scalar UDF) when Serial Plan fires for a UDF-related reason (TSQLUserDefinedFunctionsNotParallelizable, CLRUserDefinedFunctionRequiresDataAccess, or CouldNotGenerateValidParallelPlan) — the Serial Plan warning already explains the issue. Updated test to assert the new suppression behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- b2: Rework Serial Plan benefit formula per Joe's proposal: (cpu * (DOP - 1) / DOP) / elapsed * 100, assuming DOP 4 when unknown. No benefit when StatementSubTreeCost < 1. Drops the unnecessary CPU-bound branch and the 50% cap for non-CPU-bound cases. - b7: Excessive Memory Grant warning now notes when an adaptive join executed as Nested Loops at runtime — the grant was sized for the hash alternative that wasn't used. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Review — Joe feedback b2/b3/b6/b7
What it does
- Rule 3 Serial Plan: hardcodes "MAXDOP is set to 1 using a query hint" for the
MaxDOPSetToOne+ explicit-hint path (PlanAnalyzer.cs:215). - Rule 6 Scalar UDF: suppressed when
NonParallelPlanReasonis UDF-related (PlanAnalyzer.cs:634–640). - Rule 9 Excessive Memory Grant: appends a note when an adaptive join ran as Nested Loops (PlanAnalyzer.cs:257–260 + new
HasAdaptiveJoinChoseNestedLoopat 1451). - BenefitScorer Serial Plan: new formula
(cpu * (DOP-1) / DOP) / elapsed * 100, DOP=4, cost≥1 gate (BenefitScorer.cs:53–66).
Good
- Targets
dev, notmain. - Rule 6 test was updated to match the new suppression behavior (PlanAnalyzerTests.cs:97).
- The adaptive-join traversal is contained and side-effect free.
Needs attention
- Dashboard/Lite sync: Rules 3, 6, 9 and the Serial Plan benefit formula all changed. PerformanceMonitor Dashboard and Lite keep their own copy of this rule set — confirm the equivalent changes are landing there, otherwise the three surfaces drift.
- Duplicate
<summary>doc block at PlanAnalyzer.cs:1444–1450. See inline. - Missing test coverage for the new BenefitScorer formula and the adaptive-join memory-grant branch. See inline.
- Minor: unbraced
foreachunder a compoundifat PlanAnalyzer.cs:642. See inline.
No Avalonia/XAML changes in this diff, so the usual AvaloniaEdit/ComboBox/contrast checks don't apply.
Generated by Claude Code
| /// <summary> | ||
| /// Returns true if the plan contains an adaptive join that executed as a Nested Loop. | ||
| /// Indicates a memory grant was sized for the hash alternative but never needed. | ||
| /// </summary> |
There was a problem hiding this comment.
Duplicate <summary> doc block. The pre-existing "Finds Sort and Hash Match operators..." summary at 1444–1446 belonged to FindMemoryConsumers below. Inserting HasAdaptiveJoinChoseNestedLoop here orphaned it — now this method has two summaries and FindMemoryConsumers has none. Move the old summary back down to line 1464 (immediately above FindMemoryConsumers).
Generated by Claude Code
| // Note adaptive joins that chose Nested Loops at runtime — the grant | ||
| // was sized for a hash join that never happened. | ||
| if (stmt.RootNode != null && HasAdaptiveJoinChoseNestedLoop(stmt.RootNode)) | ||
| message += " An adaptive join in this plan executed as a Nested Loop at runtime — the memory grant was sized for the hash join alternative that wasn't used."; |
There was a problem hiding this comment.
No test covers the adaptive-join-chose-nested-loop branch. Rule09a_ExcessiveMemoryGrant_SmallGrantSuppressed (tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs:182) only asserts the suppression path. Add a fixture plan with IsAdaptive=true + ActualJoinType="NestedLoops" and an over-granted statement so the new message suffix is exercised.
Generated by Claude Code
| warning.MaxBenefitPercent = Math.Round(Math.Min(50, benefit), 1); | ||
| var benefit = ((double)cpu * (potentialDop - 1) / potentialDop) / elapsedMs * 100; | ||
| warning.MaxBenefitPercent = Math.Round(Math.Min(100, benefit), 1); | ||
| } |
There was a problem hiding this comment.
No test asserts the new formula. Existing Serial Plan test Rule03_SerialPlan_MaxDOPSetToOne (PlanAnalyzerTests.cs:53) only checks the message text, not MaxBenefitPercent. Recommend at least one test pinning the numeric output for a known cpu/elapsed pair so regressions in the formula surface — e.g. cpu≈elapsed → ~75%, cpu=0 → null/unset, subTreeCost<1 → unset.
Also worth confirming: the StatementSubTreeCost >= 1.0 gate silently drops benefit for sub-cost-1 serial plans but the warning still fires from PlanAnalyzer (which has no matching cost gate). Intentional? If so, a comment on the asymmetry would help the next reader.
Generated by Claude Code
| if (!cfg.IsRuleDisabled(6) && !serialPlanCoversUdf) | ||
| foreach (var udf in node.ScalarUdfs) | ||
| { | ||
| var type = udf.IsClrFunction ? "CLR" : "T-SQL"; |
There was a problem hiding this comment.
Style nit: the if (!cfg.IsRuleDisabled(6) && !serialPlanCoversUdf) guards a foreach with no braces. Works, but mixing a compound condition with an unbraced loop is easy to misread — wrap the foreach in braces to match the rest of the file.
Generated by Claude Code
1 participant
Summary
Four items from Joe's latest feedback on issue #215:
(cpu * (DOP-1) / DOP) / elapsed * 100, DOP=4 assumed, no benefit below cost 1. Removed the unnecessary CPU-bound branch and the 50% capTest plan
Remaining items from Joe's comment (not in this PR): b1 (bare scan benefit), b4/b5 (surface significant waits as actionable items — overlaps with a1 in the per-operator consolidation design).
🤖 Generated with Claude Code