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 4 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!
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.