workspace(perf,refactor) Bulk set-option helper + 10ms readiness polling#1040
Open
tony wants to merge 3 commits into
Open
workspace(perf,refactor) Bulk set-option helper + 10ms readiness polling#1040tony wants to merge 3 commits into
tony wants to merge 3 commits into
Conversation
…o 10ms why: Empirical microbenchmark (60-pane test suite) measured the previous 50ms interval costing ~41ms more per pane on subprocess and ~35ms more on imsg than a 10ms interval, because tmux's shell-ready latency lands in the 50-150ms band and the coarser poll missed the early-readiness window. Tightening to 10ms saves ~2.5s wall time on tmuxp's suite with no correctness change — same condition, just polled more frequently. what: - _wait_for_pane_ready: change interval default from 0.05 to 0.01
…loops why: tmuxp's workspace builder fires N round-trips per workspace load applying session/window/options_after metadata, one ``set-option`` per loop iteration. Centralising those calls behind a single helper means we have one place to swap in a pipelined dispatch (e.g. ``Server.batch()``) the moment libtmux exposes one. The helper lands first without call-site changes so behaviour parity can be verified in isolation. The original draft of this work (``libtmux-protocol`` branch, ``f4c95faa``) used ``Server.batch()`` directly. That API doesn't exist on libtmux 0.56.0 (it ships with the unmerged libtmux-protocol-engines work), and even when present its perf gain is documented as "no-op cost on subprocess and imsg" — so batching only pays off on the unshipped control_mode engine. To land the API shape against current libtmux without committing to unshipped upstream, this commit issues one ``set-option`` per item via ``server.cmd``. The helper API (mapping, target, scope_flag) is deliberately batch-shaped so the body can swap back to a single batched dispatch later without touching the call sites. what: - Add ``WorkspaceBuilder._bulk_set_options(items, *, target, scope_flag)`` mirroring ``OptionsMixin.set_option``'s bool -> "on"/"off" normalisation (libtmux/options.py:712) so loop and helper paths produce identical tmux commands. - Reuse libtmux's ``handle_option_error`` to preserve the ``OptionError`` subclasses callers already catch — the same propagation ``OptionsMixin.set_option`` does inside its own body (libtmux/options.py:712). - Empty-mapping guard avoids a zero-iteration loop. - Add two regression tests in tests/workspace/test_builder.py: ``test_bulk_set_options_propagates_unknown_option_error`` proves bad options still raise ``OptionError`` from the helper; ``test_bulk_set_options_applies_session_window_and_options_after`` exercises the helper across all three scopes (-s, -g, -w) and verifies each option lands on tmux via ``show_option`` / ``show-option -v``. Salvaged from libtmux-protocol@f4c95faa with the body rewritten to drop the unshipped ``Server.batch()`` dependency. Test docstrings updated to match the loop-based reality.
…_options why: Workspace loading dispatches one tmux ``set-option`` per loop iteration across four hot loops (session ``options``, ``global_options``, per-window ``options``, ``options_after``). The helper that ``f4c95faa`` introduced provides a single, batch-shaped entry point for these calls; this commit moves the existing call sites onto it. Today the helper is a plain loop so this is a pure refactor (same N round-trips, same observable behaviour). Once libtmux exposes a pipelined ``Server.batch()`` the helper body can swap internally and the call sites automatically benefit without further changes here. what: - Replace the session ``options`` loop in ``build`` with ``_bulk_set_options(..., scope_flag="-s")`` targeting ``session.session_id``. - Replace the ``global_options`` loop in ``build`` with ``_bulk_set_options(..., target=None, scope_flag="-g")``. - Replace the per-window ``options`` loop in ``iter_create_windows`` with ``_bulk_set_options(..., scope_flag="-w")`` targeting ``window.window_id``. - Replace the ``options_after`` loop in ``_apply_options_after`` (or equivalent post-build hook) with ``_bulk_set_options(..., scope_flag="-w")`` targeting ``window.window_id``. Salvaged from libtmux-protocol@2390ce3b. The original commit described the change as a perf win; against current libtmux 0.56.0 the helper body issues N round-trips per call site (no ``Server.batch()`` yet), so this commit is documented as a refactor and the perf framing is deferred to whenever libtmux ships a batching API.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1040 +/- ##
==========================================
- Coverage 81.96% 81.89% -0.07%
==========================================
Files 28 28
Lines 2545 2558 +13
Branches 485 487 +2
==========================================
+ Hits 2086 2095 +9
- Misses 328 330 +2
- Partials 131 133 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Summary
Three small, self-contained workspace-builder changes salvaged from a stalled experimental branch:
_wait_for_pane_readypolling tightened from 50ms to 10ms. Cuts ~2-3s off suite wall time (within run-to-run noise but consistent with commit body's claim)._bulk_set_optionshelper added — a single, batch-shaped entry point for the fourset-optionloops the builder runs (sessionoptions,global_options, per-windowoptions,options_after). Today the helper body is a plain loop, so this is a pure refactor; the call sites land on it so the body can later swap to a pipelined dispatch (e.g. a hypotheticalServer.batch()) without touching the call sites.set-optionloops.The helper exists today as forward-compat plumbing — there's no measurable perf gain from the refactor against current libtmux (0.56.0 ships only the subprocess path; batching pays off on engines libtmux hasn't released). The polling-tighten commit is the only commit that materially affects wall time.
Why this PR exists
The bulk-options helper originated on a local-only experiment branch (
libtmux-protocol) that pinned libtmux to a private worktree exposing aServer.batch()API. That experiment hasn't shipped upstream and may take time to land. Rather than let the call-site shape rot on a branch nobody can review, this PR salvages the shape against current libtmux. WhenServer.batch()(or equivalent) ships, the helper body becomes a one-line internal swap; the call sites stay as-is.The polling-tighten commit was on the same experiment branch and is independent — it lands here because it shares context.
Measured timing (3 runs salvage, 2 runs master)
Median pytest is essentially tied (~77s). Median wall favors salvage by ~2s, matching the commit body's "~2.5s wall time" claim. One salvage run hit a tmux-state flake with 2 reruns — same flake potential exists on master, just didn't surface in these particular master runs.
Per-commit breakdown
5cef952a—perf(workspace[wait]) tighten _wait_for_pane_ready polling interval to 10msOne-line change to the
intervaldefault in_wait_for_pane_ready. Tighter loop catches the early-readiness window (50-150ms tmux shell-ready latency) more reliably; the previous 50ms interval was missing the leading edge by definition.6ccb0590—workspace(feat[builder]) add _bulk_set_options helper for set-option loopsAdds
WorkspaceBuilder._bulk_set_options(items, *, target, scope_flag). MirrorsOptionsMixin.set_option'sTrue/False -> "on"/"off"normalisation so loop and helper paths produce identical tmux commands. Reuses libtmux'shandle_option_errorto preserve theOptionErrorsubclasses callers already catch.Body is a plain loop today. The API shape (mapping + scope flag + optional target) is deliberately batch-shaped so the body can swap to a pipelined dispatch when libtmux exposes one.
Adds two regression tests (
test_bulk_set_options_propagates_unknown_option_error,test_bulk_set_options_applies_session_window_and_options_after).9d4e4f3f—workspace(refactor[builder]) route set-option loops through _bulk_set_optionsConverts the four hot loops to call the helper. Pure refactor — same N round-trips per call site, same observable behaviour. The original commit framed this as a perf change; rewritten here as a refactor since
Server.batch()isn't on libtmux 0.56.0 and the original commit body itself notes batching is "no-op cost on subprocess and imsg" anyway. Perf framing deferred to whenever libtmux ships a batching API.Test plan
uv run ruff check . --fix --show-fixes— cleanuv run ruff format .— cleanuv run mypy— clean (124 source files)uv run py.test --reruns 0 -vvv— 799 passed, 2 skipped (master had 797 + 2 new regression tests)just build-docs— succeeds