perf: make beat timelines usable at 14k beats - #600
Draft
FelipeDefensor wants to merge 20 commits into
Draft
Conversation
Replace the open-coded `compute_is_first_in_measure = False / True` toggles in `crop`, `clear`, `deserialize_components`, `restore_state`, `delete_components`, and `fill_with_beats` with a single `is_first_in_measure_computation_paused` context manager on `BeatTLComponentManager`. The ctxmgr saves and restores the prior value so nested uses don't accidentally re-enable computation halfway through an outer bulk op. Also fixes a typo in `delete_components` that wrote to a nonexistent attribute `update_is_first_in_measure` instead of restoring `compute_is_first_in_measure` to True — meaning every bulk delete left the flag stuck at False until the next site happened to flip it.
The CSV beat import called `timeline.recalculate_measures()` per row, and each `create_component` ran its own recalculate when `compute_is_first_in_measure` was True. Both are O(n) over the running beat count, making the import O(n^2). A ~14k-beat CSV could take several minutes. Pause `is_first_in_measure` computation on the component manager for the whole second pass, then call `recalculate_measures()` and `update_is_first_in_measure_of_subsequent_beats(0)` once at the end — the same pattern already used by `fill_with_beats` and `restore_state`. Measured: 13932-beat import drops from "did not finish" to ~25 s.
Replace O(N) list.index with O(log N) bisect_left on the sorted components list. This is the load-bearing fix for slow single-beat insertion: get_beat_index is hit per beat from Beat.metric_position during update_metric_fraction_dicts, making the per-insert path O(N^2). With bisect it becomes O(N log N). Times are unique (validate_unique_position in pointlike), so bisecting by time uniquely identifies the beat.
Four call sites still did list.index(beat) directly on the components list, each O(N), turning per-insert work into O(N^2): - BeatTLComponentManager.create_component (line 73): beat_index - BeatTLComponentManager.delete_component (line 94): component_idx - BeatTimeline.is_first_in_measure (line 384) - BeatTimelineUI.should_display_measure_number (line 212) All now use get_beat_index, inheriting the bisect-based O(log N) lookup from the previous commit.
BeatTimelineUI.on_add called recalculate_measures after create_component, but the backend already triggers it inside BeatTLComponentManager.create_component when compute_is_first_in_measure is True. Removing the redundant call roughly halves per-insert work (recalculate_measures clears metric-position caches and rebuilds metric_fraction_dicts).
Asserts that adding a single beat in the middle of a 1000-beat timeline finishes in under 2 s. Pre-bisect, the per-insert path was O(N^2) and easily took multiple seconds on N=1000; with the bisect-based get_beat_index the same insert completes in well under a second.
`beats_that_start_measures` is strictly increasing (built from `itertools.accumulate(beats_in_measure[:-1])`), so the linear scan in `get_measure_index` can be replaced with `bisect`. With beat_pattern=[4] the list grows to ~N/4 entries, so the original O(M) lookup × the ~1.3·N call sites per single-beat insert was the remaining O(N²) factor after the earlier `get_beat_index` bisect work. Profile delta at N=5000, single middle-insert (offscreen platform): - get_measure_index cumtime: 319 ms (6722 calls) -> ~5 ms - update_metric_fraction_dicts: 290 ms -> 107 ms - total insert: 684 ms -> 396 ms Bench wall times (median, --runs 3, offscreen): - N=5000: 451 ms -> 250 ms - N=10000: 1454 ms -> 474 ms Scaling 5k -> 10k drops from 3.3× to 1.9×, confirming the residual quadratic is gone. The original code had a quirky fallback branch returning offset `1` for `beat_index > btsm[-1]` regardless of how far past, so the new branch preserves that behavior verbatim instead of returning the "natural" `beat_index - btsm[-1]`.
Standalone script for profiling/benchmarking single-beat-insert into N-beat timelines. Sets QT_QPA_PLATFORM before any Qt import so the underlying platform plugin (minimal / offscreen / windows) can be swapped on each invocation, and supports an optional cProfile dump. Used to size the impact of the recent get_beat_index and get_measure_index bisect work; kept in tree as the next perf step will need it again.
…sure_of_subsequent_beats The function ran a final `update_metric_fraction_dicts()` after the bulk `is_first_in_measure` flip loop. `metric_position` depends on `get_beat_index` / `get_measure_index` / `measure_numbers` / `beats_in_measure` — none of which involve the `is_first_in_measure` flag — so the bulk flips can never invalidate the dicts. Every caller of this method either rebuilt the dicts beforehand (create_component, restore_state, CSV parser) or rebuilds them afterward separately (delete_component's UI follow-up), so the in-function rebuild was always dead work. Profile call site at N=5000 was the second of two `update_metric_fraction_dicts` invocations per insert (~54 ms each). Bench wall times (median, --runs 3, offscreen): - N=5000: 250 ms -> 152 ms - N=10000: 474 ms -> 384 ms
…e flip loop `update_is_first_in_measure_of_subsequent_beats` previously routed each flip through `set_component_data`, which fires `Post.TIMELINE_COMPONENT_SET_DATA_DONE` and triggers a UI cascade (`element.update` -> `update_is_first_in_measure` -> body.set_position + `update_label`). At N=5000 a middle-insert flips ~N/2 beats, so that's ~1249 redundant UI updates per insert. Callers of this method already follow up with `Post.BEAT_TIMELINE_MEASURE_NUMBER_CHANGE_DONE`, which drives a single bulk UI pass over the same beats, so the per-set posts were pure duplicate work. The loop now mutates `is_first_in_measure` directly on the Beat. This is safe because: - `is_first_in_measure` isn't in `Beat.SERIALIZABLE`, so the component hash doesn't depend on it. - `metric_position` doesn't read it, so the metric_fraction dicts can't go stale. - The undo/redo snapshot doesn't capture it either; it's recomputed via this same method on restore. `BeatTimelineUI.on_measure_number_change_done` was previously enough because the per-set cascade refreshed each flipped beat's body. Without that cascade, the bulk handler must do both, so it now calls `update_is_first_in_measure` (which also calls `update_label`) instead of `update_label` alone. Bench wall times (median, --runs 3, offscreen): - N=5000: 152 ms -> 93 ms - N=10000: 384 ms -> 333 ms Cumulative since the bisect cascade started: - N=5000: 451 ms -> 93 ms (4.8x) - N=10000: 1454 ms -> 333 ms (4.4x)
`Post.BEAT_TIMELINE_MEASURE_NUMBER_CHANGE_DONE` is consumed as a beat-index (the receiver slices `self[start_index:]` over the beat-UI collection), but `create_component` was posting `measure_index - 1`, which has measure-index semantics and lands far before the new beat in a typical timeline. At N=5000 / 4-beats-per-measure that meant the bulk UI pass refreshed ~4376 beats instead of the ~2500 actually past the insertion point. Pass the new beat's index directly. Median middle-insert at: N=5000: 93ms -> 68ms (-27%) N=10000: 333ms -> 204ms (-39%) Other call sites of this post (`set_measure_number`, `unforce_display_measure_number`, `set_beat_amount_in_measure`) also pass a measure_index; they're not on the hot path and their slice is over-broad-but-correct, so leave them for a follow-up.
…action_dicts `update_metric_fraction_dicts` rebuilt three dicts from scratch each call, with 5000+ `beat.metric_position` cache misses per insert at N=5000 (5000 bisect_left into components + 5000 bisect into beats_that_start_measures, plus the dict ops). Rewrite as a two-pointer pass that walks `beats_that_start_measures` in parallel with `enumerate(self.components)`, so the per-beat work is O(1) instead of O(log N + log M). Populate `_cached_metric_position` inline while we're at it — `recalculate_measures` cleared the caches just before this call, so otherwise every subsequent `metric_position` access starts cold. The trailing __sort_* passes still need to run: `measure_numbers` can be non-monotonic (e.g. pickup measures number their entries as [1, 3, 0, 2, 4, 5]), so insertion order in beat-index order is not guaranteed to match sorted-key order. Tried skipping the sort first and broke `test_changing_attributes` in the musicxml parser, which exercises exactly that case. Wall-clock at N=5000 (offscreen, 5 runs): before: median 72ms, min 69ms after: median 61ms, min 59ms (-16% median) Cumulative since perf work started: 451ms -> 60ms at N=5k (7.5x).
…inserted index Thread `start_index` through `recalculate_measures` -> `clear_cached_metric_positions` + `update_beats_that_start_measures` -> `update_metric_fraction_dicts`. `create_component` computes the new beat index up-front (cheap bisect on times — works before the recalc fixes beats_in_measure) and passes it in. Upstream beats (0..K-1) are structurally unaffected by an insert at K: their measure / beat-in-measure / beat-count don't change. So `update_metric_fraction_dicts(start_index=K)` keeps their cached positions and dict entries untouched, and only: - pops stale `time_to_metric_fraction` entries for beats[K:], - filters their old beat-list / time-list entries out of `metric_fraction_to_beat_dict` / `metric_fraction_to_time`, - re-runs the two-pointer pass from K to populate fresh entries. For middle-insert at K = N/2 this halves the work. For *end-insert* (K = N-1, the live-tap workload) the loop runs once and there are no stale entries to clean up — that's the load-bearing case. Wall-clock at N=13000, end-insert (offscreen, 5 runs): before: ~250ms (extrapolated from N=10k middle perf) after: median 55ms, min 54ms Wall-clock at N=10000, end-insert: median 31ms (under 50ms live-tap budget) Wall-clock at N=5000, end-insert: median 16ms Middle-insert at N=5000 is unchanged at 60ms median — the UI bulk pass dominates there, and the recalc work was already halved by two-pointer in the prior commit. Middle-insert in long timelines will need the planned batch-mode toggle to skip per-tap recalc. Default start_index=0 preserves the full-rebuild behavior for all other callers (restore_state, deserialize, set_measure_number, etc.).
The default middle-insert measures worst-case structural work. End- insert measures the live-tap workload (appending while playing), which is what the per-beat latency budget actually targets.
Lets a command behave as a toggle action — the registered QAction becomes checkable, and the callback can read the current state via commands.get_qaction(name).isChecked(). Needed for the upcoming beat-timeline batch-mode toggle.
…g bulk tap
Adds command `timeline.beat.toggle_recalculate_measures` (Ctrl+Shift+B,
plus an entry in the beat-timeline context menu). When the user pauses,
the `compute_is_first_in_measure` flag on the component manager is
cleared — `create_component` already gates its whole recalc block on
that flag, so each tap reduces from O(N) (bisect + dict rebuild + UI
bulk pass) to O(log N) (just the sorted insert). On resume the callback
runs a single catch-up `recalculate_measures()` plus a UI refresh post,
restoring the full state.
Per-tap cost at N=13000, middle position:
no batch: ~250-300ms
batch on: median 39ms (capped by undo-snapshot serialization,
not recalc work)
catch-up: ~415ms one-time on resume
The undo snapshot is now the floor — to push per-tap below 10ms we'd
need to defer per-tap APP_STATE_RECORD too, then bundle the whole
batch into one undoable. Out of scope for this commit.
The cm flag is the source of truth for the toggle. The QAction's check
state is synced to match, regardless of whether the user invoked via
menu click (Qt pre-toggles the action) or via shortcut / programmatic
`commands.execute()` (action state unchanged). Tidies a pre-existing
typo in `BeatTimelineUIContextMenu.items` (`MenuItemKind` -> `MenuItemKind.COMMAND`)
incidentally — both work because the loose match falls through to
`add_action`, but the explicit form matches the rest of the codebase.
Toggles measure-recalc-pause before tapping and resumes after; prints the resume catch-up cost separately so we can see what the user actually pays during a batch tap workflow.
…ntext menu
`TimelineUIContextMenu.add_action` rewires the QAction's triggered
signal to call `commands.execute(name, self.timeline_ui)` — passing
the clicked timeline as an extra positional arg. The arg flows through
`on_timeline_command` and lands as a second positional on the
callback. Before the fix, invoking the toggle from the timeline
context menu raised:
TypeError: BeatTimelineUI.on_toggle_recalculate_measures()
takes 1 positional argument but 2 were given
Accept `*_` to swallow it. Add a regression test that triggers the
context-menu action directly.
Also surfaces the toggle on the beat-timeline toolbar (icon
MediaPlaybackPause; the QToolButton renders pressed when checked,
which is the visual feedback for paused mode).
…ition Zoom on a 14k-beat timeline was ~112 ms per step (offscreen, median of 5). Profile showed all time in `update_time_on_elements` -> 14000 × `BeatUI.update_position`, with two hotspots: 1. `self.x` and `self.height` were each evaluated twice per call (once for body, once for label). Each `x` access runs `time_x_converter.get_x_by_time` + a `get_data` on the component, each `height` runs a `get_data`. 2. `self.label.set_position` ran for every beat, even though with `beat_pattern>=2` the label is hidden for most beats. `set_position` calls Qt's `setPos` plus `boundingRect()` per call, which dominates per-beat cost despite being wasted work for invisible items. Cache x/height into locals, then guard the label update behind `isVisible()`. Zoom doesn't change visibility, so a hidden label stays hidden; when `update_label` later runs (e.g. after `is_first_in_measure` flips) it sets text and position together. Bench (N=14000, offscreen, --runs 5): - before: median 112.71 ms - after : median 66.51 ms (-41%) Also adds scripts/bench_beat_zoom.py to mirror the existing bench_beat_insert.py harness.
… name v0.6.5 shipped the same idea under a different name: `BeatTLComponentManager.suppressing_is_first_in_measure()`. This branch introduced `is_first_in_measure_computation_paused()` independently, so merging 0.6.5 forward into dev would leave two context managers doing the same job. Take the released name and implementation verbatim, including the `Iterator[bool]` yield of the flag's entry value — that lets a caller tell whether it owns the recomputation or an outer block will do it once at the end, which the version here lacked. Every call site on this branch ignores the yielded value, so the behaviour is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. Opening this early to get a read on scope and on the one design question at the bottom before I polish it.
Beat timelines get slow well before they get large. A ~14k-beat timeline (a full movement tapped by hand) makes tapping, zooming and CSV import unusable. This branch works through the hot paths.
What's slow, and why
Nearly all of it traces back to two things:
Beat.metric_positionresolved itself withcomponents.index(beat), which copies the component list and scans it, andrecalculate_measures()rebuilt three metric-fraction dicts from scratch on every single beat insert. Both are O(n) per beat, so every bulk path was O(n²).Measurements
All offscreen, median of 5 runs, via the two bench scripts added here.
Middle-insert without batch mode is unchanged at N=5000 (~60 ms) — the UI bulk pass dominates there, not the recalculation.
How it's organised
Each commit is standalone with its own rationale and numbers. Roughly:
get_beat_index/get_measure_index, and routing.indexlookups through them.update_metric_fraction_dictsthat populates the position cache inline, then an incremental path scoped to the inserted index so an end-insert only walks the tail.Poststorm and two redundant rebuilds.x/heightinupdate_positionand skippingset_positionfor labels that are hidden anyway (withbeat_pattern >= 2most labels are hidden, andset_positioncosts a QtsetPos+boundingRect()each).Two things worth reviewing separately
1. The batch-mode toggle is a feature, not a perf fix.
timeline.beat.toggle_recalculate_measures(Ctrl+Shift+B, context menu + toolbar) lets the user suspend measure recalculation while tapping a passage and pay one catch-up on resume. It's here because it's the only thing that makes middle-insert bearable on long timelines, but it adds UI surface and a newcommands.register(checkable=True)capability. Happy to split it into its own PR if you'd rather review it on its own.At N=13000 the per-tap cost in batch mode is 39 ms, and that floor is the undo-snapshot serialization, not recalculation. Going below ~10 ms means deferring per-tap
APP_STATE_RECORDand bundling the batch into a single undoable — deliberately out of scope here.2. The context manager overlaps with v0.6.5. This branch grew a context manager for suspending
compute_is_first_in_measure; v0.6.5 shipped the same idea assuppressing_is_first_in_measure(). The last commit drops this branch's name and takes the released one verbatim, so merging 0.6.5 forward intodevshould be a no-op for that hunk rather than a conflict. Worth a sanity check that I took the right direction.Not included
refactor/beat-suppression-contextmanagerhas two related fixes that aren't here —crop()/clear()leaving a stale measure structure behind, and a duplicate recalculation on every deletion. They're bug fixes rather than perf work and sit on a v0.6.4 base, so they belong in their own PR.Testing
Full suite green on this base (1849 passed, 10 skipped). Adds a perf regression guard for middle-insert and a 5-second-timeout guard on a 1000-beat CSV import — the latter fails outright on
dev.scripts/bench_beat_insert.pyandscripts/bench_beat_zoom.pyare dev harnesses, not tests; they take--position end|middle,--batchand--runs.