Skip to content

Commit 72d3657

Browse files
committed
gh-154032: Fix differential flamegraph scaling
1 parent a1d5804 commit 72d3657

5 files changed

Lines changed: 94 additions & 19 deletions

File tree

Lib/profiling/sampling/_flamegraph_assets/flamegraph.js

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,11 @@ function getDisplayName(moduleName, filename) {
9999
return filename;
100100
}
101101

102+
function samplesToMilliseconds(samples, data) {
103+
const intervalUsec = data?.stats?.sample_interval_usec ?? 1000;
104+
return samples * intervalUsec / 1000;
105+
}
106+
102107
function selectFlamegraphData() {
103108
const baseData = isShowingElided ? elidedFlamegraphData : normalData;
104109

@@ -246,12 +251,13 @@ function setupLogos() {
246251
// Status Bar
247252
// ============================================================================
248253

249-
function updateStatusBar(nodeData, rootValue) {
254+
function updateStatusBar(nodeData, data) {
250255
const funcname = resolveString(nodeData.funcname) || resolveString(nodeData.name) || "--";
251256
const filename = resolveString(nodeData.filename) || "";
252257
const moduleName = resolveString(nodeData.module) || "";
253258
const lineno = nodeData.lineno;
254-
const timeMs = (nodeData.value / 1000).toFixed(2);
259+
const timeMs = samplesToMilliseconds(nodeData.value, data).toFixed(2);
260+
const rootValue = data.value;
255261
const percent = rootValue > 0 ? ((nodeData.value / rootValue) * 100).toFixed(1) : "0.0";
256262

257263
const brandEl = document.getElementById('status-brand');
@@ -313,9 +319,9 @@ function createPythonTooltip(data) {
313319
.style("opacity", 0);
314320
}
315321

316-
const timeMs = (d.data.value / 1000).toFixed(2);
322+
const timeMs = samplesToMilliseconds(d.data.value, data).toFixed(2);
317323
const selfSamples = d.data.self || 0;
318-
const selfMs = (selfSamples / 1000).toFixed(2);
324+
const selfMs = samplesToMilliseconds(selfSamples, data).toFixed(2);
319325
const percentage = ((d.data.value / data.value) * 100).toFixed(2);
320326
const relativePercentage = Math.min(100, ((d.data.value / (zoomedNodeValue ?? data.value)) * 100)).toFixed(2);
321327
const calls = d.data.calls || 0;
@@ -399,9 +405,9 @@ function createPythonTooltip(data) {
399405
// Differential stats section
400406
let diffSection = "";
401407
if (d.data.diff !== undefined && d.data.baseline !== undefined) {
402-
const baselineSelf = (d.data.baseline / 1000).toFixed(2);
403-
const currentSelf = ((d.data.self_time || 0) / 1000).toFixed(2);
404-
const diffMs = (d.data.diff / 1000).toFixed(2);
408+
const baselineSelf = samplesToMilliseconds(d.data.baseline, data).toFixed(2);
409+
const currentSelf = samplesToMilliseconds(d.data.self_time || 0, data).toFixed(2);
410+
const diffMs = samplesToMilliseconds(d.data.diff, data).toFixed(2);
405411
const diffPct = d.data.diff_pct;
406412
const sign = d.data.diff >= 0 ? "+" : "";
407413
const diffClass = d.data.diff > 0 ? "regression" : (d.data.diff < 0 ? "improvement" : "neutral");
@@ -499,7 +505,7 @@ function createPythonTooltip(data) {
499505
.style("opacity", 1);
500506

501507
// Update status bar
502-
updateStatusBar(d.data, data.value);
508+
updateStatusBar(d.data, data);
503509
};
504510

505511
pythonTooltip.hide = function () {

Lib/profiling/sampling/stack_collector.py

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ def export(self, filename):
7070
class FlamegraphCollector(StackTraceCollector):
7171
def __init__(self, *args, **kwargs):
7272
super().__init__(*args, **kwargs)
73-
self.stats = {}
73+
self.stats = {"sample_interval_usec": self.sample_interval_usec}
7474
self._root = {"samples": 0, "children": {}, "threads": set()}
7575
self._total_samples = 0
7676
self._sample_count = 0 # Track actual number of samples (not thread traces)
@@ -550,16 +550,19 @@ def _convert_to_flamegraph_format(self):
550550
current_stats = self._aggregate_path_samples(self._root)
551551
baseline_stats = self._aggregate_path_samples(self._baseline_collector._root)
552552

553-
# Scale baseline values to make them comparable, accounting for both
554-
# sample count differences and sample interval differences.
553+
# Scale baseline samples to the number of samples in the current
554+
# profile. The sample interval is only needed when converting samples
555+
# to time for display.
555556
baseline_total = self._baseline_collector._total_samples
556557
if baseline_total > 0 and self._total_samples > 0:
557-
current_time = self._total_samples * self.sample_interval_usec
558-
baseline_time = baseline_total * self._baseline_collector.sample_interval_usec
559-
scale = current_time / baseline_time
558+
scale = self._total_samples / baseline_total
560559
elif baseline_total > 0:
561-
# Current profile is empty - use interval-based scale for elided display
562-
scale = self.sample_interval_usec / self._baseline_collector.sample_interval_usec
560+
# Express baseline samples in units of the current sample interval
561+
# for the elided display.
562+
scale = (
563+
self._baseline_collector.sample_interval_usec
564+
/ self.sample_interval_usec
565+
)
563566
else:
564567
scale = 1.0
565568

@@ -653,6 +656,7 @@ def _build_elided_flamegraph(self, baseline_stats, scale):
653656
if not self._extract_elided_nodes(baseline_data, path=()):
654657
return None
655658

659+
self._scale_flamegraph_values(baseline_data, scale)
656660
self._add_elided_metadata(baseline_data, baseline_stats, scale, path=())
657661

658662
# Merge only profiling metadata, not thread-level stats
@@ -666,6 +670,13 @@ def _build_elided_flamegraph(self, baseline_stats, scale):
666670

667671
return baseline_data
668672

673+
def _scale_flamegraph_values(self, node, scale):
674+
"""Express flamegraph values in units of the current sample interval."""
675+
node["value"] = node.get("value", 0) * scale
676+
node["self"] = node.get("self", 0) * scale
677+
for child in node.get("children", ()):
678+
self._scale_flamegraph_values(child, scale)
679+
669680
def _extract_elided_nodes(self, node, path):
670681
"""Remove non-elided nodes and recalculate values bottom-up."""
671682
if not node:

Lib/test/test_profiling/test_sampling_profiler/mocks.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,20 +93,24 @@ def __repr__(self):
9393
return f"MockAwaitedInfo(thread_id={self.thread_id}, awaited_by={len(self.awaited_by)} tasks)"
9494

9595

96-
def make_diff_collector_with_mock_baseline(baseline_samples):
96+
def make_diff_collector_with_mock_baseline(
97+
baseline_samples, *, baseline_interval=1000, current_interval=1000
98+
):
9799
"""Create a DiffFlamegraphCollector with baseline injected directly,
98100
skipping the binary round-trip that _load_baseline normally does."""
99101
from profiling.sampling.stack_collector import (
100102
DiffFlamegraphCollector,
101103
FlamegraphCollector,
102104
)
103105

104-
baseline = FlamegraphCollector(1000)
106+
baseline = FlamegraphCollector(baseline_interval)
105107
for sample in baseline_samples:
106108
baseline.collect(sample)
107109

108110
# Path is unused since we inject _baseline_collector directly;
109111
# use __file__ as a dummy path that passes the existence check.
110-
diff = DiffFlamegraphCollector(1000, baseline_binary_path=__file__)
112+
diff = DiffFlamegraphCollector(
113+
current_interval, baseline_binary_path=__file__
114+
)
111115
diff._baseline_collector = baseline
112116
return diff

Lib/test/test_profiling/test_sampling_profiler/test_collectors.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,7 @@ def test_flamegraph_collector_basic(self):
505505
self.assertIn("func1 (file.py:10)", resolve_name(child, strings))
506506
self.assertEqual(child["value"], 1)
507507
self.assertEqual(child["self"], 1) # leaf: all time is self
508+
self.assertEqual(data["stats"]["sample_interval_usec"], 1000)
508509

509510
def test_flamegraph_collector_export(self):
510511
"""Test flamegraph HTML export functionality."""
@@ -1556,6 +1557,57 @@ def test_diff_flamegraph_scale_factor(self):
15561557
self.assertAlmostEqual(func1_node["diff"], 0.0)
15571558
self.assertAlmostEqual(func1_node["diff_pct"], 0.0)
15581559

1560+
def test_diff_flamegraph_scale_factor_with_different_intervals(self):
1561+
"""Scale factor normalizes profiles sampled at different rates."""
1562+
frames = [
1563+
MockInterpreterInfo(0, [
1564+
MockThreadInfo(1, [MockFrameInfo("file.py", 10, "func1")])
1565+
])
1566+
]
1567+
1568+
diff = make_diff_collector_with_mock_baseline(
1569+
[frames] * 10,
1570+
baseline_interval=1000,
1571+
current_interval=10000,
1572+
)
1573+
diff.collect(frames)
1574+
1575+
data = diff._convert_to_flamegraph_format()
1576+
self.assertAlmostEqual(data["stats"]["baseline_scale"], 0.1)
1577+
self.assertEqual(data["stats"]["sample_interval_usec"], 10000)
1578+
self.assertAlmostEqual(data["baseline"], 1.0)
1579+
self.assertEqual(data["self_time"], 1)
1580+
self.assertAlmostEqual(data["diff"], 0.0)
1581+
self.assertAlmostEqual(data["diff_pct"], 0.0)
1582+
1583+
def test_diff_flamegraph_elided_scale_with_different_intervals(self):
1584+
"""Elided values use the current profile's sample units."""
1585+
baseline_frames = [
1586+
MockInterpreterInfo(0, [
1587+
MockThreadInfo(1, [MockFrameInfo("file.py", 10, "old_func")])
1588+
])
1589+
]
1590+
current_frames = [
1591+
MockInterpreterInfo(0, [
1592+
MockThreadInfo(1, [MockFrameInfo("file.py", 20, "new_func")])
1593+
])
1594+
]
1595+
1596+
diff = make_diff_collector_with_mock_baseline(
1597+
[baseline_frames] * 10,
1598+
baseline_interval=1000,
1599+
current_interval=10000,
1600+
)
1601+
diff.collect(current_frames)
1602+
1603+
data = diff._convert_to_flamegraph_format()
1604+
elided = data["stats"]["elided_flamegraph"]
1605+
self.assertEqual(elided["stats"]["sample_interval_usec"], 10000)
1606+
self.assertAlmostEqual(elided["value"], 1.0)
1607+
self.assertAlmostEqual(elided["self"], 1.0)
1608+
self.assertAlmostEqual(elided["baseline"], 1.0)
1609+
self.assertAlmostEqual(elided["diff"], -1.0)
1610+
15591611
def test_diff_flamegraph_elided_stacks(self):
15601612
"""Paths in baseline but not current produce elided stacks."""
15611613
baseline_frames_1 = [
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix differential flamegraphs incorrectly reporting changes when the baseline
2+
and current profiles use different sampling rates.

0 commit comments

Comments
 (0)