Skip to content

Commit 78b8bd5

Browse files
committed
gh-142374: Fix recursive function cumulative over-counting in sampling profiler
The sampling profiler counted every frame occurrence in a stack for cumulative statistics. For recursive functions appearing N times in a stack, this meant counting N instead of 1, causing cumul% to exceed 100%. A function recursing 500 deep in every sample would show 50000% cumulative presence. The fix tracks seen locations per sample using a reused set, ensuring each unique (filename, lineno, funcname) is counted once per sample. This matches the expected semantics: cumul% represents the percentage of samples where a function appeared on the stack, not the sum of all frame occurrences.
1 parent c5b3722 commit 78b8bd5

File tree

7 files changed

+304
-18
lines changed

7 files changed

+304
-18
lines changed

Lib/profiling/sampling/heatmap_collector.py

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,10 @@ def __init__(self, *args, **kwargs):
477477
# File index (populated during export)
478478
self.file_index = {}
479479

480+
# Reusable set for deduplicating line locations within a single sample.
481+
# This avoids over-counting recursive functions in cumulative stats.
482+
self._seen_lines = set()
483+
480484
def set_stats(self, sample_interval_usec, duration_sec, sample_rate, error_rate=None, missed_samples=None, **kwargs):
481485
"""Set profiling statistics to include in heatmap output.
482486
@@ -509,6 +513,7 @@ def process_frames(self, frames, thread_id):
509513
thread_id: Thread ID for this stack trace
510514
"""
511515
self._total_samples += 1
516+
self._seen_lines.clear()
512517

513518
# Count each line in the stack and build call graph
514519
for i, frame_info in enumerate(frames):
@@ -519,7 +524,13 @@ def process_frames(self, frames, thread_id):
519524

520525
# frames[0] is the leaf - where execution is actually happening
521526
is_leaf = (i == 0)
522-
self._record_line_sample(filename, lineno, funcname, is_leaf=is_leaf)
527+
line_key = (filename, lineno)
528+
count_cumulative = line_key not in self._seen_lines
529+
if count_cumulative:
530+
self._seen_lines.add(line_key)
531+
532+
self._record_line_sample(filename, lineno, funcname, is_leaf=is_leaf,
533+
count_cumulative=count_cumulative)
523534

524535
# Build call graph for adjacent frames
525536
if i + 1 < len(frames):
@@ -537,11 +548,13 @@ def _is_valid_frame(self, filename, lineno):
537548

538549
return True
539550

540-
def _record_line_sample(self, filename, lineno, funcname, is_leaf=False):
551+
def _record_line_sample(self, filename, lineno, funcname, is_leaf=False,
552+
count_cumulative=True):
541553
"""Record a sample for a specific line."""
542554
# Track cumulative samples (all occurrences in stack)
543-
self.line_samples[(filename, lineno)] += 1
544-
self.file_samples[filename][lineno] += 1
555+
if count_cumulative:
556+
self.line_samples[(filename, lineno)] += 1
557+
self.file_samples[filename][lineno] += 1
545558

546559
# Track self/leaf samples (only when at top of stack)
547560
if is_leaf:

Lib/profiling/sampling/live_collector/collector.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ def __init__(
190190
# Trend tracking (initialized after colors are set up)
191191
self._trend_tracker = None
192192

193+
self._seen_locations = set()
194+
193195
@property
194196
def elapsed_time(self):
195197
"""Get the elapsed time, frozen when finished."""
@@ -285,13 +287,16 @@ def process_frames(self, frames, thread_id=None):
285287

286288
# Get per-thread data if tracking per-thread
287289
thread_data = self._get_or_create_thread_data(thread_id) if thread_id is not None else None
290+
self._seen_locations.clear()
288291

289292
# Process each frame in the stack to track cumulative calls
290293
for frame in frames:
291294
location = (frame.filename, frame.lineno, frame.funcname)
292-
self.result[location]["cumulative_calls"] += 1
293-
if thread_data:
294-
thread_data.result[location]["cumulative_calls"] += 1
295+
if location not in self._seen_locations:
296+
self._seen_locations.add(location)
297+
self.result[location]["cumulative_calls"] += 1
298+
if thread_data:
299+
thread_data.result[location]["cumulative_calls"] += 1
295300

296301
# The top frame gets counted as an inline call (directly executing)
297302
top_location = (frames[0].filename, frames[0].lineno, frames[0].funcname)

Lib/profiling/sampling/pstats_collector.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,21 @@ def __init__(self, sample_interval_usec, *, skip_idle=False):
1616
lambda: collections.defaultdict(int)
1717
)
1818
self.skip_idle = skip_idle
19+
self._seen_locations = set()
1920

2021
def _process_frames(self, frames):
2122
"""Process a single thread's frame stack."""
2223
if not frames:
2324
return
2425

26+
self._seen_locations.clear()
27+
2528
# Process each frame in the stack to track cumulative calls
2629
for frame in frames:
2730
location = (frame.filename, frame.lineno, frame.funcname)
28-
self.result[location]["cumulative_calls"] += 1
31+
if location not in self._seen_locations:
32+
self._seen_locations.add(location)
33+
self.result[location]["cumulative_calls"] += 1
2934

3035
# The top frame gets counted as an inline call (directly executing)
3136
top_location = (frames[0].filename, frames[0].lineno, frames[0].funcname)

Lib/test/test_profiling/test_sampling_profiler/test_collectors.py

Lines changed: 195 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ def test_pstats_collector_with_extreme_intervals_and_empty_data(self):
8585
# Should still process the frames
8686
self.assertEqual(len(collector.result), 1)
8787

88-
# Test collecting duplicate frames in same sample
88+
# Test collecting duplicate frames in same sample (recursive function)
8989
test_frames = [
9090
MockInterpreterInfo(
9191
0, # interpreter_id
@@ -94,17 +94,17 @@ def test_pstats_collector_with_extreme_intervals_and_empty_data(self):
9494
1,
9595
[
9696
MockFrameInfo("file.py", 10, "func1"),
97-
MockFrameInfo("file.py", 10, "func1"), # Duplicate
97+
MockFrameInfo("file.py", 10, "func1"), # Duplicate (recursion)
9898
],
9999
)
100100
],
101101
)
102102
]
103103
collector = PstatsCollector(sample_interval_usec=1000)
104104
collector.collect(test_frames)
105-
# Should count both occurrences
105+
# Should count only once per sample to avoid over-counting recursive functions
106106
self.assertEqual(
107-
collector.result[("file.py", 10, "func1")]["cumulative_calls"], 2
107+
collector.result[("file.py", 10, "func1")]["cumulative_calls"], 1
108108
)
109109

110110
def test_pstats_collector_single_frame_stacks(self):
@@ -1201,3 +1201,194 @@ def test_flamegraph_collector_per_thread_gc_percentage(self):
12011201
self.assertEqual(collector.per_thread_stats[2]["gc_samples"], 1)
12021202
self.assertEqual(collector.per_thread_stats[2]["total"], 6)
12031203
self.assertAlmostEqual(per_thread_stats[2]["gc_pct"], 10.0, places=1)
1204+
1205+
1206+
class TestRecursiveFunctionHandling(unittest.TestCase):
1207+
"""Tests for correct handling of recursive functions in cumulative stats."""
1208+
1209+
def test_pstats_collector_recursive_function_single_sample(self):
1210+
"""Test that recursive functions are counted once per sample, not per occurrence."""
1211+
collector = PstatsCollector(sample_interval_usec=1000)
1212+
1213+
# Simulate a recursive function appearing 5 times in one sample
1214+
recursive_frames = [
1215+
MockInterpreterInfo(
1216+
0,
1217+
[
1218+
MockThreadInfo(
1219+
1,
1220+
[
1221+
MockFrameInfo("test.py", 10, "recursive_func"),
1222+
MockFrameInfo("test.py", 10, "recursive_func"),
1223+
MockFrameInfo("test.py", 10, "recursive_func"),
1224+
MockFrameInfo("test.py", 10, "recursive_func"),
1225+
MockFrameInfo("test.py", 10, "recursive_func"),
1226+
],
1227+
)
1228+
],
1229+
)
1230+
]
1231+
collector.collect(recursive_frames)
1232+
1233+
location = ("test.py", 10, "recursive_func")
1234+
# Should count as 1 cumulative call (present in 1 sample), not 5
1235+
self.assertEqual(collector.result[location]["cumulative_calls"], 1)
1236+
# Direct calls should be 1 (top of stack)
1237+
self.assertEqual(collector.result[location]["direct_calls"], 1)
1238+
1239+
def test_pstats_collector_recursive_function_multiple_samples(self):
1240+
"""Test cumulative counting across multiple samples with recursion."""
1241+
collector = PstatsCollector(sample_interval_usec=1000)
1242+
1243+
# Sample 1: recursive function at depth 3
1244+
sample1 = [
1245+
MockInterpreterInfo(
1246+
0,
1247+
[
1248+
MockThreadInfo(
1249+
1,
1250+
[
1251+
MockFrameInfo("test.py", 10, "recursive_func"),
1252+
MockFrameInfo("test.py", 10, "recursive_func"),
1253+
MockFrameInfo("test.py", 10, "recursive_func"),
1254+
],
1255+
)
1256+
],
1257+
)
1258+
]
1259+
# Sample 2: recursive function at depth 2
1260+
sample2 = [
1261+
MockInterpreterInfo(
1262+
0,
1263+
[
1264+
MockThreadInfo(
1265+
1,
1266+
[
1267+
MockFrameInfo("test.py", 10, "recursive_func"),
1268+
MockFrameInfo("test.py", 10, "recursive_func"),
1269+
],
1270+
)
1271+
],
1272+
)
1273+
]
1274+
# Sample 3: recursive function at depth 4
1275+
sample3 = [
1276+
MockInterpreterInfo(
1277+
0,
1278+
[
1279+
MockThreadInfo(
1280+
1,
1281+
[
1282+
MockFrameInfo("test.py", 10, "recursive_func"),
1283+
MockFrameInfo("test.py", 10, "recursive_func"),
1284+
MockFrameInfo("test.py", 10, "recursive_func"),
1285+
MockFrameInfo("test.py", 10, "recursive_func"),
1286+
],
1287+
)
1288+
],
1289+
)
1290+
]
1291+
1292+
collector.collect(sample1)
1293+
collector.collect(sample2)
1294+
collector.collect(sample3)
1295+
1296+
location = ("test.py", 10, "recursive_func")
1297+
# Should count as 3 cumulative calls (present in 3 samples)
1298+
# Not 3+2+4=9 which would be the buggy behavior
1299+
self.assertEqual(collector.result[location]["cumulative_calls"], 3)
1300+
self.assertEqual(collector.result[location]["direct_calls"], 3)
1301+
1302+
def test_pstats_collector_mixed_recursive_and_nonrecursive(self):
1303+
"""Test a call stack with both recursive and non-recursive functions."""
1304+
collector = PstatsCollector(sample_interval_usec=1000)
1305+
1306+
# Stack: main -> foo (recursive x3) -> bar
1307+
frames = [
1308+
MockInterpreterInfo(
1309+
0,
1310+
[
1311+
MockThreadInfo(
1312+
1,
1313+
[
1314+
MockFrameInfo("test.py", 50, "bar"), # top of stack
1315+
MockFrameInfo("test.py", 20, "foo"), # recursive
1316+
MockFrameInfo("test.py", 20, "foo"), # recursive
1317+
MockFrameInfo("test.py", 20, "foo"), # recursive
1318+
MockFrameInfo("test.py", 10, "main"), # bottom
1319+
],
1320+
)
1321+
],
1322+
)
1323+
]
1324+
collector.collect(frames)
1325+
1326+
# bar: 1 cumulative (in stack), 1 direct (top)
1327+
self.assertEqual(collector.result[("test.py", 50, "bar")]["cumulative_calls"], 1)
1328+
self.assertEqual(collector.result[("test.py", 50, "bar")]["direct_calls"], 1)
1329+
1330+
# foo: 1 cumulative (counted once despite 3 occurrences), 0 direct
1331+
self.assertEqual(collector.result[("test.py", 20, "foo")]["cumulative_calls"], 1)
1332+
self.assertEqual(collector.result[("test.py", 20, "foo")]["direct_calls"], 0)
1333+
1334+
# main: 1 cumulative, 0 direct
1335+
self.assertEqual(collector.result[("test.py", 10, "main")]["cumulative_calls"], 1)
1336+
self.assertEqual(collector.result[("test.py", 10, "main")]["direct_calls"], 0)
1337+
1338+
def test_pstats_collector_cumulative_percentage_cannot_exceed_100(self):
1339+
"""Test that cumulative percentage stays <= 100% even with deep recursion."""
1340+
collector = PstatsCollector(sample_interval_usec=1000000) # 1 second for easy math
1341+
1342+
# Collect 10 samples, each with recursive function at depth 100
1343+
for _ in range(10):
1344+
frames = [
1345+
MockInterpreterInfo(
1346+
0,
1347+
[
1348+
MockThreadInfo(
1349+
1,
1350+
[MockFrameInfo("test.py", 10, "deep_recursive")] * 100,
1351+
)
1352+
],
1353+
)
1354+
]
1355+
collector.collect(frames)
1356+
1357+
location = ("test.py", 10, "deep_recursive")
1358+
# Cumulative calls should be 10 (number of samples), not 1000
1359+
self.assertEqual(collector.result[location]["cumulative_calls"], 10)
1360+
1361+
# Verify stats calculation gives correct percentage
1362+
collector.create_stats()
1363+
stats = collector.stats[location]
1364+
# stats format: (direct_calls, cumulative_calls, total_time, cumulative_time, callers)
1365+
cumulative_calls = stats[1]
1366+
self.assertEqual(cumulative_calls, 10)
1367+
1368+
def test_pstats_collector_different_lines_same_function_counted_separately(self):
1369+
"""Test that different line numbers in same function are tracked separately."""
1370+
collector = PstatsCollector(sample_interval_usec=1000)
1371+
1372+
# Function with multiple line numbers (e.g., different call sites within recursion)
1373+
frames = [
1374+
MockInterpreterInfo(
1375+
0,
1376+
[
1377+
MockThreadInfo(
1378+
1,
1379+
[
1380+
MockFrameInfo("test.py", 15, "func"), # line 15
1381+
MockFrameInfo("test.py", 12, "func"), # line 12
1382+
MockFrameInfo("test.py", 15, "func"), # line 15 again
1383+
MockFrameInfo("test.py", 10, "func"), # line 10
1384+
],
1385+
)
1386+
],
1387+
)
1388+
]
1389+
collector.collect(frames)
1390+
1391+
# Each unique (file, line, func) should be counted once
1392+
self.assertEqual(collector.result[("test.py", 15, "func")]["cumulative_calls"], 1)
1393+
self.assertEqual(collector.result[("test.py", 12, "func")]["cumulative_calls"], 1)
1394+
self.assertEqual(collector.result[("test.py", 10, "func")]["cumulative_calls"], 1)

Lib/test/test_profiling/test_sampling_profiler/test_integration.py

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,16 +118,17 @@ def test_recursive_function_call_counting(self):
118118
self.assertIn(fib_key, collector.stats)
119119
self.assertIn(main_key, collector.stats)
120120

121-
# Fibonacci should have many calls due to recursion
121+
# Fibonacci: counted once per sample, not per occurrence
122122
fib_stats = collector.stats[fib_key]
123123
direct_calls, cumulative_calls, tt, ct, callers = fib_stats
124124

125-
# Should have recorded multiple calls (9 total appearances in samples)
126-
self.assertEqual(cumulative_calls, 9)
127-
self.assertGreater(tt, 0) # Should have some total time
128-
self.assertGreater(ct, 0) # Should have some cumulative time
125+
# Should count 3 (present in 3 samples), not 9 (total occurrences)
126+
self.assertEqual(cumulative_calls, 3)
127+
self.assertEqual(direct_calls, 3) # Top of stack in all samples
128+
self.assertGreater(tt, 0)
129+
self.assertGreater(ct, 0)
129130

130-
# Main should have fewer calls
131+
# Main should also have 3 cumulative calls (in all 3 samples)
131132
main_stats = collector.stats[main_key]
132133
main_direct_calls, main_cumulative_calls = main_stats[0], main_stats[1]
133134
self.assertEqual(main_direct_calls, 0) # Never directly executing

0 commit comments

Comments
 (0)