-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
gh-152721: Fix quadratic RLE replay time in the profiling binary reader #152722
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
1dac106
beb5980
3e71061
6594318
70a2e41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix quadratic replay time in the :mod:`profiling.sampling` binary reader when a | ||
| profile's run-length-encoded samples alternate thread status. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ | |
| /* Progress callback frequency */ | ||
| #define PROGRESS_CALLBACK_INTERVAL 1000 | ||
|
|
||
| /* Cap per-batch RLE samples to bound the timestamp list (gh-151378) */ | ||
| #define MAX_RLE_BATCH_SAMPLES 8192 | ||
|
|
||
| /* ============================================================================ | ||
| * BINARY READER IMPLEMENTATION | ||
| * ============================================================================ */ | ||
|
|
@@ -947,21 +950,6 @@ emit_sample(RemoteDebuggingState *state, PyObject *collector, | |
| return 0; | ||
| } | ||
|
|
||
| /* Helper to trim timestamp list and emit batch. Returns 0 on success, -1 on error. */ | ||
| static int | ||
| emit_batch(RemoteDebuggingState *state, PyObject *collector, | ||
| uint64_t thread_id, uint32_t interpreter_id, uint8_t status, | ||
| const uint32_t *frame_indices, size_t stack_depth, | ||
| BinaryReader *reader, PyObject *timestamps_list, Py_ssize_t actual_size) | ||
| { | ||
| /* Trim list to actual size */ | ||
| if (PyList_SetSlice(timestamps_list, actual_size, PyList_GET_SIZE(timestamps_list), NULL) < 0) { | ||
| return -1; | ||
| } | ||
| return emit_sample(state, collector, thread_id, interpreter_id, status, | ||
| frame_indices, stack_depth, reader, timestamps_list); | ||
| } | ||
|
|
||
| /* Helper to invoke progress callback, returns -1 on error */ | ||
| static inline int | ||
| invoke_progress_callback(PyObject *callback, Py_ssize_t current, uint32_t total) | ||
|
|
@@ -1095,17 +1083,18 @@ binary_reader_replay(BinaryReader *reader, PyObject *collector, PyObject *progre | |
| ts->prev_timestamp += delta; | ||
|
|
||
| /* Start new batch on first sample or status change */ | ||
| if (i == 0 || status != batch_status) { | ||
| if (i == 0 || status != batch_status | ||
| || batch_idx >= MAX_RLE_BATCH_SAMPLES) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a test that covers the |
||
| if (timestamps_list) { | ||
| int rc = emit_batch(state, collector, thread_id, interpreter_id, | ||
| batch_status, ts->current_stack, ts->current_stack_depth, | ||
| reader, timestamps_list, batch_idx); | ||
| int rc = emit_sample(state, collector, thread_id, interpreter_id, | ||
| batch_status, ts->current_stack, ts->current_stack_depth, | ||
| reader, timestamps_list); | ||
| Py_DECREF(timestamps_list); | ||
| if (rc < 0) { | ||
| return -1; | ||
| } | ||
| } | ||
| timestamps_list = PyList_New(count - i); | ||
| timestamps_list = PyList_New(0); | ||
| if (!timestamps_list) { | ||
| return -1; | ||
| } | ||
|
|
@@ -1118,14 +1107,20 @@ binary_reader_replay(BinaryReader *reader, PyObject *collector, PyObject *progre | |
| Py_DECREF(timestamps_list); | ||
| return -1; | ||
| } | ||
| PyList_SET_ITEM(timestamps_list, batch_idx++, ts_obj); | ||
| int append_rc = PyList_Append(timestamps_list, ts_obj); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that the list is built to its exact size, the |
||
| Py_DECREF(ts_obj); | ||
| if (append_rc < 0) { | ||
| Py_DECREF(timestamps_list); | ||
| return -1; | ||
| } | ||
| batch_idx++; | ||
| } | ||
|
|
||
| /* Emit final batch */ | ||
| if (timestamps_list) { | ||
| int rc = emit_batch(state, collector, thread_id, interpreter_id, | ||
| batch_status, ts->current_stack, ts->current_stack_depth, | ||
| reader, timestamps_list, batch_idx); | ||
| int rc = emit_sample(state, collector, thread_id, interpreter_id, | ||
| batch_status, ts->current_stack, ts->current_stack_depth, | ||
| reader, timestamps_list); | ||
| Py_DECREF(timestamps_list); | ||
| if (rc < 0) { | ||
| return -1; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This nicely checks the batching, but the old code produced the same batches (just slower), so this would pass before the fix too, no? It doesn't really guard against the quadratic behavior coming back. Fine to keep since a timing-based test would be flaky, just noting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point — it's really a correctness guard for the append path rather than a timing guard (which would be flaky, as you note), so I kept it. Thanks!