Skip to content

Commit 1b3020e

Browse files
committed
gh-154062: Stop normalizing differential flamegraph duration
1 parent e2118b0 commit 1b3020e

4 files changed

Lines changed: 68 additions & 38 deletions

File tree

‎Lib/profiling/sampling/stack_collector.py‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -619,16 +619,16 @@ def _convert_to_flamegraph_format(self):
619619
current_stats = self._aggregate_path_samples(self._root)
620620
baseline_stats = self._aggregate_path_samples(self._baseline_collector._root)
621621

622-
# Scale baseline values to make them comparable, accounting for both
623-
# sample count differences and sample interval differences.
622+
# Express baseline samples in units of the current sample interval.
623+
# Do not normalize by total profile duration: doing so makes unchanged
624+
# functions appear different when another function becomes faster or
625+
# slower.
624626
baseline_total = self._baseline_collector._total_samples
625-
if baseline_total > 0 and self._total_samples > 0:
626-
current_time = self._total_samples * self.sample_interval_usec
627-
baseline_time = baseline_total * self._baseline_collector.sample_interval_usec
628-
scale = current_time / baseline_time
629-
elif baseline_total > 0:
630-
# Current profile is empty - use interval-based scale for elided display
631-
scale = self.sample_interval_usec / self._baseline_collector.sample_interval_usec
627+
if baseline_total > 0:
628+
scale = (
629+
self._baseline_collector.sample_interval_usec
630+
/ self.sample_interval_usec
631+
)
632632
else:
633633
scale = 1.0
634634

‎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: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1584,7 +1584,8 @@ def test_diff_flamegraph_changed_functions(self):
15841584
])
15851585
]
15861586

1587-
# Baseline: 2 samples, current: 4, scale = 2.0
1587+
# Baseline: 2 samples, current: 4. Profiles are compared in absolute
1588+
# time rather than normalized to the same total duration.
15881589
diff = make_diff_collector_with_mock_baseline(
15891590
[hot_leaf_sample, cold_leaf_sample]
15901591
)
@@ -1594,28 +1595,28 @@ def test_diff_flamegraph_changed_functions(self):
15941595

15951596
data = diff._convert_to_flamegraph_format()
15961597
strings = data.get("strings", [])
1597-
self.assertAlmostEqual(data["stats"]["baseline_scale"], 2.0)
1598+
self.assertAlmostEqual(data["stats"]["baseline_scale"], 1.0)
15981599

15991600
children = data.get("children", [])
16001601
hot_node = find_child_by_name(children, strings, "hot_leaf")
16011602
cold_node = find_child_by_name(children, strings, "cold_leaf")
16021603
self.assertIsNotNone(hot_node)
16031604
self.assertIsNotNone(cold_node)
16041605

1605-
# hot_leaf regressed (+50%)
1606-
self.assertAlmostEqual(hot_node["baseline"], 2.0)
1606+
# hot_leaf regressed (+200%)
1607+
self.assertAlmostEqual(hot_node["baseline"], 1.0)
16071608
self.assertEqual(hot_node["self_time"], 3)
1608-
self.assertAlmostEqual(hot_node["diff"], 1.0)
1609-
self.assertAlmostEqual(hot_node["diff_pct"], 50.0)
1609+
self.assertAlmostEqual(hot_node["diff"], 2.0)
1610+
self.assertAlmostEqual(hot_node["diff_pct"], 200.0)
16101611

1611-
# cold_leaf improved (-50%)
1612-
self.assertAlmostEqual(cold_node["baseline"], 2.0)
1612+
# cold_leaf is unchanged
1613+
self.assertAlmostEqual(cold_node["baseline"], 1.0)
16131614
self.assertEqual(cold_node["self_time"], 1)
1614-
self.assertAlmostEqual(cold_node["diff"], -1.0)
1615-
self.assertAlmostEqual(cold_node["diff_pct"], -50.0)
1615+
self.assertAlmostEqual(cold_node["diff"], 0.0)
1616+
self.assertAlmostEqual(cold_node["diff_pct"], 0.0)
16161617

1617-
def test_diff_flamegraph_scale_factor(self):
1618-
"""Scale factor adjusts when sample counts differ."""
1618+
def test_diff_flamegraph_does_not_normalize_duration(self):
1619+
"""A longer current run is compared in absolute time."""
16191620
baseline_frames = [
16201621
MockInterpreterInfo(0, [
16211622
MockThreadInfo(1, [
@@ -1630,15 +1631,37 @@ def test_diff_flamegraph_scale_factor(self):
16301631
diff.collect(baseline_frames)
16311632

16321633
data = diff._convert_to_flamegraph_format()
1633-
self.assertAlmostEqual(data["stats"]["baseline_scale"], 4.0)
1634+
self.assertAlmostEqual(data["stats"]["baseline_scale"], 1.0)
16341635

16351636
children = data.get("children", [])
16361637
self.assertEqual(len(children), 1)
16371638
func1_node = children[0]
16381639
self.assertEqual(func1_node["self_time"], 4)
1639-
self.assertAlmostEqual(func1_node["baseline"], 4.0)
1640-
self.assertAlmostEqual(func1_node["diff"], 0.0)
1641-
self.assertAlmostEqual(func1_node["diff_pct"], 0.0)
1640+
self.assertAlmostEqual(func1_node["baseline"], 1.0)
1641+
self.assertAlmostEqual(func1_node["diff"], 3.0)
1642+
self.assertAlmostEqual(func1_node["diff_pct"], 300.0)
1643+
1644+
def test_diff_flamegraph_scale_factor_uses_sample_intervals(self):
1645+
"""Baseline samples are converted to current sample units."""
1646+
frames = [
1647+
MockInterpreterInfo(0, [
1648+
MockThreadInfo(1, [MockFrameInfo("file.py", 10, "func1")])
1649+
])
1650+
]
1651+
1652+
diff = make_diff_collector_with_mock_baseline(
1653+
[frames] * 10,
1654+
baseline_interval=1000,
1655+
current_interval=10000,
1656+
)
1657+
diff.collect(frames)
1658+
1659+
data = diff._convert_to_flamegraph_format()
1660+
self.assertAlmostEqual(data["stats"]["baseline_scale"], 0.1)
1661+
self.assertAlmostEqual(data["baseline"], 1.0)
1662+
self.assertEqual(data["self_time"], 1)
1663+
self.assertAlmostEqual(data["diff"], 0.0)
1664+
self.assertAlmostEqual(data["diff_pct"], 0.0)
16421665

16431666
def test_diff_flamegraph_elided_stacks(self):
16441667
"""Paths in baseline but not current produce elided stacks."""
@@ -2117,7 +2140,8 @@ def test_diff_flamegraph_load_baseline(self):
21172140
make_frame("file.py", 20, "caller"),
21182141
])])]
21192142

2120-
# Baseline: 2 samples, current: 4, scale = 2.0
2143+
# Baseline: 2 samples, current: 4. Profiles are compared in absolute
2144+
# time rather than normalized to the same total duration.
21212145
bin_file = tempfile.NamedTemporaryFile(suffix=".bin", delete=False)
21222146
self.addCleanup(close_and_unlink, bin_file)
21232147

@@ -2147,25 +2171,25 @@ def test_diff_flamegraph_load_baseline(self):
21472171
strings = data.get("strings", [])
21482172

21492173
self.assertTrue(data["stats"]["is_differential"])
2150-
self.assertAlmostEqual(data["stats"]["baseline_scale"], 2.0)
2174+
self.assertAlmostEqual(data["stats"]["baseline_scale"], 1.0)
21512175

21522176
children = data.get("children", [])
21532177
hot_node = find_child_by_name(children, strings, "hot_leaf")
21542178
cold_node = find_child_by_name(children, strings, "cold_leaf")
21552179
self.assertIsNotNone(hot_node)
21562180
self.assertIsNotNone(cold_node)
21572181

2158-
# hot_leaf regressed (+50%)
2159-
self.assertAlmostEqual(hot_node["baseline"], 2.0)
2182+
# hot_leaf regressed (+200%)
2183+
self.assertAlmostEqual(hot_node["baseline"], 1.0)
21602184
self.assertEqual(hot_node["self_time"], 3)
2161-
self.assertAlmostEqual(hot_node["diff"], 1.0)
2162-
self.assertAlmostEqual(hot_node["diff_pct"], 50.0)
2185+
self.assertAlmostEqual(hot_node["diff"], 2.0)
2186+
self.assertAlmostEqual(hot_node["diff_pct"], 200.0)
21632187

2164-
# cold_leaf improved (-50%)
2165-
self.assertAlmostEqual(cold_node["baseline"], 2.0)
2188+
# cold_leaf is unchanged
2189+
self.assertAlmostEqual(cold_node["baseline"], 1.0)
21662190
self.assertEqual(cold_node["self_time"], 1)
2167-
self.assertAlmostEqual(cold_node["diff"], -1.0)
2168-
self.assertAlmostEqual(cold_node["diff_pct"], -50.0)
2191+
self.assertAlmostEqual(cold_node["diff"], 0.0)
2192+
self.assertAlmostEqual(cold_node["diff_pct"], 0.0)
21692193

21702194
def test_jsonl_collector_export_exact_output(self):
21712195
jsonl_out = tempfile.NamedTemporaryFile(delete=False)
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Stop normalizing differential flamegraph baselines to the duration of the
2+
current profile.

0 commit comments

Comments
 (0)