Skip to content

Fix operator self-time through Compute Scalar children + one-decimal benefit % (#215 C5, C2) #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikdarlingdata merged 1 commit into from
Apr 22, 2026
Merged

Fix operator self-time through Compute Scalar children + one-decimal benefit % (#215 C5, C2) #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,9 @@ private static string FormatBytes(double bytes)
return $"{bytes / (1024L * 1024 * 1024):N1} GB";
}

private static string FormatBenefitPercent(double pct) =>
pct >= 100 ? $"{pct:N0}" : $"{pct:N1}";
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge case: pct in the 99.95–99.99 range will format as "100.0" under N1 (rounds up) but the ternary only switches to N0 at >= 100. You'll get strings like "up to 100.0%" which reads weird next to the "100" case the branch is trying to produce. If the intent was "show integer only at exactly 100", consider rounding before the compare: var r = Math.Round(pct, 1); return r >= 100 ? $"{r:N0}" : $"{r:N1}";.

Generated by Claude Code


#endregion

#region Node Selection & Properties Panel
Expand Down Expand Up @@ -1737,7 +1740,7 @@ private void ShowPropertiesPanel(PlanNode node)
: w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF";
var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) };
var planWarnHeader = w.MaxBenefitPercent.HasValue
? $"\u26A0 {w.WarningType} \u2014 up to {w.MaxBenefitPercent:N0}% benefit"
? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
: $"\u26A0 {w.WarningType}";
warnPanel.Children.Add(new TextBlock
{
Expand Down Expand Up @@ -1819,7 +1822,7 @@ private void ShowPropertiesPanel(PlanNode node)
: w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF";
var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) };
var nodeWarnHeader = w.MaxBenefitPercent.HasValue
? $"\u26A0 {w.WarningType} \u2014 up to {w.MaxBenefitPercent:N0}% benefit"
? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
: $"\u26A0 {w.WarningType}";
warnPanel.Children.Add(new TextBlock
{
Expand Down
2 changes: 1 addition & 1 deletion src/PlanViewer.App/PlanViewer.App.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<ApplicationManifest>app.manifest</ApplicationManifest>
<ApplicationIcon>EDD.ico</ApplicationIcon>
<AvaloniaUseCompiledBindingsByDefault>true</AvaloniaUseCompiledBindingsByDefault>
<Version>1.7.1</Version>
<Version>1.7.2</Version>
<Authors>Erik Darling</Authors>
<Company>Darling Data LLC</Company>
<Product>Performance Studio</Product>
Expand Down
4 changes: 2 additions & 2 deletions src/PlanViewer.Core/Output/HtmlExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ private static void WriteWaitStatsCard(StringBuilder sb, StatementResult stmt, b
{
var barPct = maxWait > 0 ? (double)w.WaitTimeMs / maxWait * 100 : 0;
var benefitTag = benefitLookup.TryGetValue(w.WaitType, out var pct)
? $" <span class=\"warn-benefit\">up to {pct:N0}%</span>"
? $" <span class=\"warn-benefit\">up to {(pct >= 100 ? pct.ToString("N0") : pct.ToString("N1"))}%</span>"
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The >= 100 ? N0 : N1 ternary is now duplicated inline in HtmlExporter (x2), TextFormatter (x2), and Index.razor (x2), while PlanViewerControl has a FormatBenefitPercent helper for the same logic. Consider hoisting a single BenefitFormatter.Format(double pct) (or extension) into PlanViewer.Core so all six call sites agree — currently a future tweak (e.g. changing the cutoff to >= 99.95) has to be made in six places and could drift.

Generated by Claude Code

: "";
sb.AppendLine("<div class=\"wait-row\">");
sb.AppendLine($"<span class=\"wait-type\">{Encode(w.WaitType)}</span>");
Expand Down Expand Up @@ -458,7 +458,7 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt)
sb.AppendLine($"<span class=\"warn-op\">{Encode(w.Operator)}</span>");
sb.AppendLine($"<span class=\"warn-type\">{Encode(w.Type)}</span>");
if (w.MaxBenefitPercent.HasValue)
sb.AppendLine($"<span class=\"warn-benefit\">up to {w.MaxBenefitPercent:N0}% benefit</span>");
sb.AppendLine($"<span class=\"warn-benefit\">up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit</span>");
sb.AppendLine($"<span class=\"warn-msg\">{Encode(w.Message)}</span>");
if (!string.IsNullOrEmpty(w.ActionableFix))
sb.AppendLine($"<span class=\"warn-fix\">{Encode(w.ActionableFix)}</span>");
Expand Down
4 changes: 2 additions & 2 deletions src/PlanViewer.Core/Output/TextFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public static void WriteText(AnalysisResult result, TextWriter writer)
foreach (var w in stmt.WaitStats.OrderByDescending(w => w.WaitTimeMs))
{
var benefitTag = benefitLookup.TryGetValue(w.WaitType, out var pct)
? $" (up to {pct:N0}% benefit)"
? $" (up to {(pct >= 100 ? pct.ToString("N0") : pct.ToString("N1"))}% benefit)"
: "";
writer.WriteLine($" {w.WaitType}: {w.WaitTimeMs:N0}ms{benefitTag}");
}
Expand Down Expand Up @@ -169,7 +169,7 @@ public static void WriteText(AnalysisResult result, TextWriter writer)
foreach (var w in sortedWarnings)
{
var benefitTag = w.MaxBenefitPercent.HasValue
? $" (up to {w.MaxBenefitPercent:N0}% benefit)"
? $" (up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit)"
: "";
writer.WriteLine($" [{w.Severity}] {w.Type}{benefitTag}: {EscapeNewlines(w.Message)}");
if (!string.IsNullOrEmpty(w.ActionableFix))
Expand Down
41 changes: 30 additions & 11 deletions src/PlanViewer.Core/Services/PlanAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@

if (unsnifffedParams.Count > 0)
{
var hasRecompile = stmt.StatementText.Contains("RECOMPILE", StringComparison.OrdinalIgnoreCase);

Check warning on line 356 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test 

Dereference of a possibly null reference.

Check warning on line 356 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test 

Dereference of a possibly null reference.
if (!hasRecompile)
{
var names = string.Join(", ", unsnifffedParams.Select(p => p.Name));
Expand Down Expand Up @@ -1204,7 +1204,7 @@
// Rule 28: Row Count Spool — NOT IN with nullable column
// Pattern: Row Count Spool with high rewinds, child scan has IS NULL predicate,
// and statement text contains NOT IN
if (!cfg.IsRuleDisabled(28) && node.PhysicalOp.Contains("Row Count Spool"))

Check warning on line 1207 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test 

Dereference of a possibly null reference.

Check warning on line 1207 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test 

Dereference of a possibly null reference.
{
var rewinds = node.HasActualStats ? (double)node.ActualRewinds : node.EstimateRewinds;
if (rewinds > 10000 && HasNotInPattern(node, stmt))
Expand Down Expand Up @@ -1593,26 +1593,45 @@
}

/// <summary>
/// Serial row mode self-time: subtract all direct children's elapsed.
/// Exchange children are skipped through to their real child.
/// Serial row mode self-time: subtract all direct children's effective elapsed.
/// Pass-through operators (Compute Scalar, etc.) don't carry runtime stats —
/// look through them to the first descendant that does. Exchange children
/// use max-child elapsed because exchange times are unreliable.
/// </summary>
private static long GetSerialOwnElapsed(PlanNode node)
{
var totalChildElapsed = 0L;
foreach (var child in node.Children)
{
var childElapsed = child.ActualElapsedMs;

// Exchange operators have unreliable times — skip to their child
if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0)
childElapsed = child.Children.Max(c => c.ActualElapsedMs);

totalChildElapsed += childElapsed;
}
totalChildElapsed += GetEffectiveChildElapsedMs(child);

return Math.Max(0, node.ActualElapsedMs - totalChildElapsed);
}

/// <summary>
/// Returns the elapsed time a child contributes to its parent's subtree.
/// Looks through pass-through operators (Compute Scalar, Parallelism exchange)
/// that don't carry reliable runtime stats.
/// </summary>
private static long GetEffectiveChildElapsedMs(PlanNode child)
{
// Exchange operators: unreliable times, use max child
if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0)
return child.Children.Max(GetEffectiveChildElapsedMs);
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a scoring-rule change — GetSerialOwnElapsed feeds Rules 7, 10, 11, 12, 32, 34 plus eager index spool / filter / nested loops / row estimate scorers per the PR body. The equivalent fix needs to land in PerformanceMonitor's Dashboard and Lite copies of the analyzer or the three products will disagree on the same plan (Joe's node-6 Hash Spill would report 95% in Dashboard/Lite but 17.4% here).

Generated by Claude Code


// Child has its own stats: use them
if (child.ActualElapsedMs > 0)
return child.ActualElapsedMs;

Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change for Parallelism children worth calling out: the old code did child.Children.Max(c => c.ActualElapsedMs), which would return 0 if the exchange's direct child was itself a Compute Scalar. The new recursive Max(GetEffectiveChildElapsedMs) now looks through that, which is probably what you want, but it changes the reported self-time for any operator sitting above a Parallelism → Compute Scalar → Real Op chain. Make sure the 20260415_1.sqlplan regression check you ran exercises that shape.

Generated by Claude Code

// No stats (Compute Scalar and similar): look through to descendants
if (child.Children.Count == 0)
return 0;

var sum = 0L;
foreach (var grandchild in child.Children)
sum += GetEffectiveChildElapsedMs(grandchild);
return sum;
}
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summing over all grandchildren works for the common case (Compute Scalar with a single input) but could over-subtract if a pass-through-without-stats ever has multiple input branches — the parent's elapsed includes those branches sequentially in row mode, but the two sibling branches' elapsed are not additive in wall time. Compute Scalar is always unary so this is theoretical today; worth either a comment stating the assumption or a guard that only recurses when child.Children.Count == 1.

Generated by Claude Code


/// <summary>
/// Calculates a Parallelism (exchange) operator's own elapsed time.
/// Exchange times are unreliable — they accumulate wait time caused by
Expand Down
4 changes: 2 additions & 2 deletions src/PlanViewer.Web/Pages/Index.razor
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ else
@w.WaitTimeMs.ToString("N0") ms
@if (benefitPct > 0)
{
<span class="wait-benefit">up to @benefitPct.ToString("N0")%</span>
<span class="wait-benefit">up to @(benefitPct >= 100 ? benefitPct.ToString("N0") : benefitPct.ToString("N1"))%</span>
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline ternary inside the Razor expression is hard to read and duplicates the logic from FormatBenefitPercent in PlanViewerControl. Extracting to a Core helper (see HtmlExporter.cs comment) cleans this line up to @BenefitFormatter.Format(benefitPct)%.

Generated by Claude Code

}
</span>
</div>
Expand Down Expand Up @@ -346,7 +346,7 @@ else
<span class="warning-type">@w.Type</span>
@if (w.MaxBenefitPercent.HasValue)
{
<span class="warn-benefit">up to @w.MaxBenefitPercent.Value.ToString("N0")% benefit</span>
<span class="warn-benefit">up to @(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))% benefit</span>
}
<span class="warning-msg">@w.Message</span>
@if (!string.IsNullOrEmpty(w.ActionableFix))
Expand Down
Loading