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#152722tonghuaroot wants to merge 5 commits into
Conversation
| } | ||
| } | ||
| timestamps_list = PyList_New(count - i); | ||
| /* Exact-size the list; alloc+trim is O(count^2). */ |
There was a problem hiding this comment.
Nit: the comment says Exact-size the list but we build with PyList_New(0) and PyList_Append, so the list grows amortized rather than being pre-sized. Maybe reword to something like Append per element; the old alloc(count - i) + trim per batch was O(count^2). Feel free to ignore.
There was a problem hiding this comment.
Done in beb5980 — reworded to your suggestion; the list is built with per-element append, not pre-sized. Thanks!
| self.assertEqual(count, 100) | ||
| self.assert_samples_equal(samples, collector) | ||
|
|
||
| def test_rle_alternating_status_batches_correctly(self): |
There was a problem hiding this comment.
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.
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!
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).
There was a problem hiding this comment.
Wondering also about:
| if (i == 0 || status != batch_status | |
| || batch_idx >= MAX_RLE_BATCH_SAMPLES) { |
with MAX_RLE_BATCH_SAMPLES equal to 8192 (defined somewhere at the top)
I think this should also protect the memory
ref #152089
There was a problem hiding this comment.
Done in 3e71061: added MAX_RLE_BATCH_SAMPLES (8192), so a long same-status run now splits into bounded batches (the #152089 memory angle) with unchanged output. Thanks!
| /* 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) { |
There was a problem hiding this comment.
Do we have a test that covers the MAX_RLE_BATCH_SAMPLES split? With num_samples = 2000 and alternating status the cap never triggers. A run of >8192 identical-status samples should replay as two batches with the timestamps split between them.
| } | ||
| } | ||
| timestamps_list = PyList_New(count - i); | ||
| /* Append per element; the old alloc(count - i) + trim per batch was O(count^2). */ |
There was a problem hiding this comment.
Nit: this line is over 79 columns and the comment describes the old code. Maybe just drop it; the PyList_New(0) + PyList_Append pattern is clear enough on its own.
| /* Progress callback frequency */ | ||
| #define PROGRESS_CALLBACK_INTERVAL 1000 | ||
|
|
||
| /* Cap per-batch RLE samples to bound the timestamp list (gh-152089) */ |
There was a problem hiding this comment.
Nit: gh-152089 is the PR number; the issue for the writer-side bound is gh-151378. Can you point to the issue instead?
| return -1; | ||
| } | ||
| PyList_SET_ITEM(timestamps_list, batch_idx++, ts_obj); | ||
| int append_rc = PyList_Append(timestamps_list, ts_obj); |
There was a problem hiding this comment.
Now that the list is built to its exact size, the PyList_SetSlice trim in emit_batch is always a no-op, no? We could call emit_sample directly and drop actual_size.
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 (pythongh-151378) and drop the stale over-long comment.
|
Done in 70a2e41: call |
3 participants
The
STACK_REPEATbranch ofbinary_reader_replaystarted a fresh timestamplist sized to the whole remaining sample count (
PyList_New(count - i)) on everystatus change and trimmed it afterwards with
PyList_SetSlice. When theper-sample status alternates, that allocates and trims an ~
count-sized list persample, making a single repeat record
O(count**2)to replay.This builds each status run's list to its exact length with
PyList_New(0)+PyList_Append, so replay is linear. Output is unchanged: the same onecollect()call per status run with the same timestamps (emit_batch'sPyList_SetSlicetrim is now a no-op).A regression test crafts one repeat record whose status alternates every sample
and asserts it replays as N single-status batches with the right cumulative
timestamps.