Skip to content

Commit e4a2473

Browse files
gh-152721: Fix quadratic RLE replay time in the profiling binary reader (#152722)
* gh-152721: Fix quadratic RLE replay time in the profiling binary reader * gh-152721: Clarify the batch-list comment Reword per review: the list is built with append per element, not pre-sized; note the old alloc(count - i) + trim per batch was O(count^2). * Cap RLE batch size to bound the per-batch timestamp list * Move MAX_RLE_BATCH_SAMPLES comment to its own line (keep under 79 cols) * Drop the now-redundant emit_batch trim and fix the review nits The batch list is built to its exact size, so the PyList_SetSlice trim was a no-op; call emit_sample directly. Also point the cap comment at the issue (gh-151378) and drop the stale over-long comment. * gh-152721: Test replay batches beyond the RLE limit --------- Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
1 parent 8df2580 commit e4a2473

3 files changed

Lines changed: 93 additions & 24 deletions

File tree

‎Lib/test/test_profiling/test_sampling_profiler/test_binary_format.py‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import struct
88
import tempfile
99
import unittest
10+
from unittest import mock
1011
from collections import defaultdict
1112

1213
from test.support import captured_stderr
@@ -1503,6 +1504,77 @@ def test_alternating_threads_status_changes(self):
15031504
self.assertEqual(count, 100)
15041505
self.assert_samples_equal(samples, collector)
15051506

1507+
def test_rle_alternating_status_batches_correctly(self):
1508+
"""A repeat record whose status alternates every sample replays as N
1509+
single-status batches with the right cumulative timestamps."""
1510+
class BatchCollector:
1511+
def __init__(self):
1512+
self.batches = []
1513+
1514+
def collect(self, stack_frames, timestamps_us):
1515+
for interp in stack_frames:
1516+
for thread in interp.threads:
1517+
self.batches.append(
1518+
(thread.status, list(timestamps_us))
1519+
)
1520+
1521+
def export(self, filename):
1522+
pass
1523+
1524+
num_samples = 2000
1525+
frame = make_frame("rle.py", 42, "rle_func")
1526+
with tempfile.NamedTemporaryFile(suffix=".bin", delete=False) as f:
1527+
filename = f.name
1528+
self.temp_files.append(filename)
1529+
1530+
writer = BinaryCollector(filename, 1000, compression="none")
1531+
expected = []
1532+
for i in range(num_samples):
1533+
status = THREAD_STATUS_HAS_GIL if i % 2 else 0
1534+
ts = 1000 + i
1535+
expected.append((status, [ts]))
1536+
sample = [
1537+
make_interpreter(0, [make_thread(1, [frame], status)])
1538+
]
1539+
writer.collect(sample, timestamp_us=ts)
1540+
writer.export(None)
1541+
1542+
collector = BatchCollector()
1543+
with BinaryReader(filename) as reader:
1544+
count = reader.replay_samples(collector)
1545+
1546+
self.assertEqual(count, num_samples)
1547+
self.assertEqual(len(collector.batches), num_samples)
1548+
self.assertEqual(collector.batches, expected)
1549+
1550+
1551+
def test_rle_long_run_splits_batches(self):
1552+
# Construct a single repeat record larger than the writer's buffer.
1553+
num_samples = 8193
1554+
filename = self.create_binary_file([], compression="none")
1555+
data = bytearray(pathlib.Path(filename).read_bytes())
1556+
record = (struct.pack("=QIB", 1, 0, 0) # STACK_REPEAT
1557+
+ b"\x81\x40" # 8193 as a varint
1558+
+ b"\x01\x00" * num_samples) # delta=1, status=0
1559+
data[64:64] = record
1560+
struct.pack_into("=Q", data, 12, 0) # start timestamp
1561+
struct.pack_into("=Q", data, 28, num_samples)
1562+
struct.pack_into("=I", data, 36, 1) # thread count
1563+
for offset in (40, 48): # string and frame table offsets
1564+
old_offset = struct.unpack_from("=Q", data, offset)[0]
1565+
struct.pack_into("=Q", data, offset, old_offset + len(record))
1566+
struct.pack_into("=Q", data, len(data) - 24, len(data))
1567+
pathlib.Path(filename).write_bytes(data)
1568+
1569+
collector = mock.Mock()
1570+
with BinaryReader(filename) as reader:
1571+
count = reader.replay_samples(collector)
1572+
batches = [call.args[1] for call in collector.collect.call_args_list]
1573+
self.assertEqual(count, num_samples)
1574+
self.assertEqual([len(batch) for batch in batches], [8192, 1])
1575+
self.assertEqual([ts for batch in batches for ts in batch],
1576+
list(range(1, num_samples + 1)))
1577+
15061578

15071579
class TestBinaryStress(BinaryFormatTestBase):
15081580
"""Randomized stress tests for binary format."""
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix quadratic replay time in the :mod:`profiling.sampling` binary reader when a
2+
profile's run-length-encoded samples alternate thread status.

‎Modules/_remote_debugging/binary_io_reader.c‎

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@
3333
/* Progress callback frequency */
3434
#define PROGRESS_CALLBACK_INTERVAL 1000
3535

36+
/* Cap per-batch RLE samples to bound the timestamp list (gh-151378) */
37+
#define MAX_RLE_BATCH_SAMPLES 8192
38+
3639
/* ============================================================================
3740
* BINARY READER IMPLEMENTATION
3841
* ============================================================================ */
@@ -1083,21 +1086,6 @@ emit_sample(RemoteDebuggingState *state, PyObject *collector,
10831086
return 0;
10841087
}
10851088

1086-
/* Helper to trim timestamp list and emit batch. Returns 0 on success, -1 on error. */
1087-
static int
1088-
emit_batch(RemoteDebuggingState *state, PyObject *collector,
1089-
uint64_t thread_id, uint32_t interpreter_id, uint8_t status,
1090-
const uint32_t *frame_indices, size_t stack_depth,
1091-
BinaryReader *reader, PyObject *timestamps_list, Py_ssize_t actual_size)
1092-
{
1093-
/* Trim list to actual size */
1094-
if (PyList_SetSlice(timestamps_list, actual_size, PyList_GET_SIZE(timestamps_list), NULL) < 0) {
1095-
return -1;
1096-
}
1097-
return emit_sample(state, collector, thread_id, interpreter_id, status,
1098-
frame_indices, stack_depth, reader, timestamps_list);
1099-
}
1100-
11011089
/* Helper to invoke progress callback, returns -1 on error */
11021090
static inline int
11031091
invoke_progress_callback(PyObject *callback, Py_ssize_t current, uint64_t total)
@@ -1226,17 +1214,18 @@ binary_reader_replay(BinaryReader *reader, PyObject *collector, PyObject *progre
12261214
ts->prev_timestamp += delta;
12271215

12281216
/* Start new batch on first sample or status change */
1229-
if (i == 0 || status != batch_status) {
1217+
if (i == 0 || status != batch_status
1218+
|| batch_idx >= MAX_RLE_BATCH_SAMPLES) {
12301219
if (timestamps_list) {
1231-
int rc = emit_batch(state, collector, thread_id, interpreter_id,
1232-
batch_status, ts->current_stack, ts->current_stack_depth,
1233-
reader, timestamps_list, batch_idx);
1220+
int rc = emit_sample(state, collector, thread_id, interpreter_id,
1221+
batch_status, ts->current_stack, ts->current_stack_depth,
1222+
reader, timestamps_list);
12341223
Py_DECREF(timestamps_list);
12351224
if (rc < 0) {
12361225
return -1;
12371226
}
12381227
}
1239-
timestamps_list = PyList_New(count - i);
1228+
timestamps_list = PyList_New(0);
12401229
if (!timestamps_list) {
12411230
return -1;
12421231
}
@@ -1249,14 +1238,20 @@ binary_reader_replay(BinaryReader *reader, PyObject *collector, PyObject *progre
12491238
Py_DECREF(timestamps_list);
12501239
return -1;
12511240
}
1252-
PyList_SET_ITEM(timestamps_list, batch_idx++, ts_obj);
1241+
int append_rc = PyList_Append(timestamps_list, ts_obj);
1242+
Py_DECREF(ts_obj);
1243+
if (append_rc < 0) {
1244+
Py_DECREF(timestamps_list);
1245+
return -1;
1246+
}
1247+
batch_idx++;
12531248
}
12541249

12551250
/* Emit final batch */
12561251
if (timestamps_list) {
1257-
int rc = emit_batch(state, collector, thread_id, interpreter_id,
1258-
batch_status, ts->current_stack, ts->current_stack_depth,
1259-
reader, timestamps_list, batch_idx);
1252+
int rc = emit_sample(state, collector, thread_id, interpreter_id,
1253+
batch_status, ts->current_stack, ts->current_stack_depth,
1254+
reader, timestamps_list);
12601255
Py_DECREF(timestamps_list);
12611256
if (rc < 0) {
12621257
return -1;

0 commit comments

Comments
 (0)