Fix #594: per-execution row estimates on inner-loop nodes; Fix #595: no-grant memory display - #597
Merged
Merged
Conversation
… estimate EstimateRows is SQL Server's estimate for one execution. ActualRows is the total across every execution, or across every thread in a parallel plan. The node label, edge color, and minimap divided the total by the raw per-execution estimate with no adjustment for either case, so a node on the inner side of a Nested Loops join (which really does run once per outer row) looked like a huge misestimate. Adds RowEstimateHelper in PlanViewer.Core: it multiplies the estimate by ActualExecutions only when the node is on the inner side of a Nested Loops join (walking the Parent chain to the join's second child) and ActualExecutions > 0. Everywhere else, ActualExecutions counts threads in a parallel plan, not repeats, so the estimate stays at one execution. The desktop app's node label and GetLinkColorBrush, the Blazor web viewer's node label, and analyzer rules 5, 16, and 26 all had their own copy of this math and now share the one helper. The CLI had no copy to fix. Golden-master baseline regenerated; every changed warning traces to this fix (see PR body for the fixture-by-fixture diff). Fix #595: Runtime Summary showed "0 KB granted, 0 KB used (100%)" for a statement with no memory grant. Now shows "No memory grant" in the neutral color, with no percentage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
The export writes the same "N of M rows" line as the node label, so it now reads ExpectedRows, a new operator result field (expected_rows in the JSON output) set from RowEstimateHelper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 19:56
|
Reviewed. The change looks good: a single |
This was referenced Sep 28, 2026
erikdarlingdata
added a commit
to erikdarlingdata/PerformanceMonitor
that referenced
this pull request
Sep 29, 2026
…udio does (#4627 follow-up) (#4698) EstimateRows is per execution and ActualRows is a total. ActualExecutions is a real loop count only on the inner side of a Nested Loops join; in a parallel zone it counts threads. The plan node's row label, the edge colour, the minimap and analyzer rules 5, 16 and 26 divided the actual rows by ActualExecutions for every node, so outside a loop they understated the actual by the DOP. This ports the fix erikdarlingdata/PerformanceStudio#597 made for erikdarlingdata/PerformanceStudio#594. - New RowEstimateHelper in PerformanceMonitor.PlanAnalysis, with PerformanceStudio's member names. IsInnerSideOfNestedLoops walks every Nested Loops ancestor. GetExpectedRows is EstimateRows times ActualExecutions on an inner side and EstimateRows everywhere else. GetRowAccuracyRatio is ActualRows over that (1.0 for no rows against a zero estimate, double.MaxValue for rows against one), and a numeric overload holds the arithmetic. - Analyzer: rule 5 compares against the expected rows and prints "(N rows x M executions)" only on an inner side. Rule 16's outer-side detail prints the total actual rows unless the outer input is itself on an inner side. Rule 26 counts a row goal as met when the ratio is at most 1.0. - The node label prints the totals through PlanRowAccuracy.FormatActualOfExpected: N0, plus the fewest decimals (at most 4) at which the printed numbers give the printed percentage. A non-zero value never prints as zero; it is reprinted to its first significant digit. Only N formats print row counts, so no magnitude prints an exponent. A Key Lookup that ran 117 times for 1 row reads "1 of 1.128 (89%)", and an accurate operator at DOP 8 reads "100 of 100 (100%)" where it read "12 of 100 (12%)". The label's brush takes its ratio from RowEstimateHelper, with the same 0.1 and 10 thresholds. - PlanEdgeColour.ForChild takes the node and gets the expected rows from RowEstimateHelper; its numeric overload takes expected rows instead of an execution count. The viewer's edge brush and the minimap both pass the node, so an accurate operator in a parallel zone at DOP 11 or more no longer draws a Blue, LightBlue or FluoBlue edge. - PlanRowAccuracy's per-execution members (ActualRowsPerExecution, Ratio and FormatActualOfEstimate) are deleted, along with the private members only they used. - Tests: Viewer4627RowEstimateHelperTests, Viewer4627RowEstimateRulesTests, Viewer4627EdgeColourTests and Viewer4627EdgeCallerTests are new, Viewer4684RowLabelTests is rewritten for the totals form, Viewer4579Tests and Viewer4627Tests are updated, and the DOP 8 fixture eager_index_spool_plan.sqlplan comes from PerformanceStudio's tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes three related bugs. The plan viewer and the analyzer both compare an operator's actual row count to its estimate. Each one gets that comparison wrong, in a different way.
Issue #595: the Runtime Summary showed a false 100% for a query with no memory grant.
When a statement's
MemoryGrantInforeportsGrantedMemory="0", the Runtime Summary showed "0 KB granted, 0 KB used (100%)". A query with no memory grant did not use 100% of anything. The panel now shows "No memory grant", with no percentage. It uses the same neutral color as any other row that is not flagging a problem.Issue #594: the plan viewer compared a total row count to a per-execution estimate.
EstimateRowsis SQL Server's estimate for one execution.ActualRowsis the total across every execution. In a parallel plan, it is the total across every thread instead. The node label, the edge color, and the minimap divided the total by the per-execution estimate, with no adjustment for either case. An operator on the inner side of a Nested Loops join runs once per outer row. A correct per-execution estimate on that operator looked like a huge misestimate.The fix adds one function,
RowEstimateHelper, inPlanViewer.Core. It multiplies the estimate byActualExecutions, but only on the inner side of a Nested Loops join. That side is the join's second child, including a loop nested inside another loop. It also requiresActualExecutionsto be greater than 0. Everywhere else,ActualExecutionscounts threads in a parallel plan, not repeats, so the estimate stays at one execution.The desktop app's node label, edge color, and minimap now call this function. The minimap needed no direct edit. It already calls the same shared
GetLinkColorBrushmethod as the main canvas.The Blazor web viewer's node label now calls it too, through a new linked-file entry in
PlanViewer.Web.csproj. The HTML export, which the web viewer offers as a download, writes the same "N of M rows" line. It now uses the same estimate, through a newexpected_rowsfield on each operator in the analysis result. So the CLI's JSON output carries that field too. The CLI needed no code change.I verified this on
eager_index_spool_plan.sqlplan(DOP 8, every operator parallel). Node 1, the join itself, is not on the inner side. It reads "609 of 2,983 (20%)", the same before and after this fix. Nodes 4 and 5 are both on the inner side. They now read "609 of 613 (99%)" instead of the old "609 of 1 (60,900%)".The analyzer had the opposite bug, in rules 5, 16, and 26. Each rule divided the actual rows by
ActualExecutionson every node, including a node that is not on the inner side. In a parallel plan, that number is a thread count, not a real execution count. The analyzer inflated real misestimates, and it invented some that were not there.In the same fixture, Node 1 read as a 39x overestimate. The real number is 4.9x. These three rules now use the same
RowEstimateHelper, so the viewer and the analyzer agree.Which component(s) does this affect?
This also affects the web viewer,
PlanViewer.Web. This template has no checkbox for it.How was this tested?
Plan files: estimated plans were not affected. The label, edge, and minimap logic all still gate on
HasActualStats. I tested actual plans with the existing fixtures undertests/PlanViewer.Core.Tests/Plans, mainlyeager_index_spool_plan.sqlplan(DOP 8) andmemory_grant_wait_plan.sqlplan. Platform: Windows, through the headless Avalonia test session (HeadlessUi).New tests:
RowEstimateHelperTestscovers the inner-side walk. It tests a plain node, the outer child, the inner child, a descendant of the inner child, and a loop nested inside another loop. It also covers the executions guard, the zero-estimate cases, and the exact numbers fromeager_index_spool_plan.sqlplanfor Nodes 1, 4, 5, and 6. It also checks the row line in the HTML export for Nodes 1 and 4.NodeLabelRowAccuracyTestsloadseager_index_spool_plan.sqlplaninto a realPlanViewerControland reads the rendered node labels. Nodes 4 and 5 read "609 of 613 (99%)" in a neutral color, not the old "609 of 1".RuntimeSummaryMemoryGrantTestsloadsmemory_grant_wait_plan.sqlplanwithGrantedMemoryandMaxUsedMemorypatched to 0. The Memory Grant row has no percent sign and a neutral color. This fixture has a real spill elsewhere in the plan, so the color check means something. Before the fix, the spill alone painted the row as a warning, even with nothing to warn about.AnalyzerRuleGateTestscover rule 5. A non-inner node in a parallel zone no longer fires a false 39x-style warning. An inner-side node with a real mismatch still fires, with the per-execution phrasing. One more test covers rule 16: the outer child's message now uses the real actual rows, not the actual divided by the thread count. One more covers rule 26: a row goal that did not hold is no longer hidden by dividing by the thread count.PlanAnalyzerTestsconfirms thateager_index_spool_plan.sqlplanNode 1 gets no "Row Estimate Mismatch" warning after the fix.Golden-master diff: before I changed the analyzer, I ran
WarningCharacterizationTestsand confirmed the existingWarningBaseline.txtmatched HEAD. I regenerated it after the fix and reviewed every changed line. All eight changed or removed lines are "Row Estimate Mismatch" warnings. Every one has the same cause. The old code divided by a thread-summedActualExecutionson a node that is not on the inner side of a Nested Loops join.eager_index_spool_plan.sqlplanmemory_grant_wait_plan.sqlplanmemory_grant_wait_plan.sqlplanparallel-skew.sqlplanserially-parallel.sqlplanslow-multi-seek.sqlplanspill_plan.sqlplanspill_plan.sqlplanNone of the 61 committed fixtures exercises the rule 16 or rule 26 fix. One way is an outer child that is itself on the inner side of an ancestor join. Another way is a row-goal scan that is not on the inner side of a Nested Loops join in a parallel zone. So the baseline diff has no lines from those two rules. The
AnalyzerRuleGateTestsabove cover both directly, with hand-built trees.Full suite:
dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Releasepassed. 1,007 tests succeeded, 1 skipped (an existing non-Windows-only test), 0 failed.dotnet build PlanViewer.sln -c Release --no-incrementalsucceeded with 0 warnings.Checklist
dotnet build -c Debug)dotnet test)Fixes #594
Fixes #595
🤖 Generated with Claude Code
https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR