workspace(perf,refactor) Bulk set-option helper + 10ms readiness polling - #1040
workspace(perf,refactor) Bulk set-option helper + 10ms readiness polling#1040tony wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1040 +/- ##
==========================================
- Coverage 82.56% 82.50% -0.07%
==========================================
Files 31 31
Lines 2770 2783 +13
Branches 518 520 +2
==========================================
+ Hits 2287 2296 +9
- Misses 346 348 +2
- Partials 137 139 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9d4e4f3 to
c13b428
Compare
…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.
c13b428 to
8b971ed
Compare
why: Workspace loads wait on shell prompt readiness with a 50ms poll, coarser than tmux's 50-150ms shell-ready band, so readiness is noticed late. Separately, the classic builder fires one set-option per loop iteration across four hot loops (session options, global_options, per-window options, options_after) with no shared seam for a future batched dispatch. what: - Tighten _wait_for_pane_ready interval default from 0.05 to 0.01 so prompt readiness is detected sooner (same condition). - Add ClassicWorkspaceBuilder._bulk_set_options(items, *, target, scope_flag): mirrors set_option's True/False -> on/off normalization and propagates errors via handle_option_error. - Route the four set-option loops through the helper: session (-s, session_id), global (-g, None), per-window (-w, window_id), options_after (-w, window_id). - Add tests/workspace/test_builder_bulk_options.py covering scope application, on/off normalization, empty no-op, and OptionError propagation. Salvaged from PR #1040, adapted from the pre-refactor single-file builder.py onto the builder package's classic.py. The helper body issues N round-trips today; the batch-shaped API defers the perf gain to a future libtmux batching API.
|
Automated review finding. Recorded here so it is not lost. Nothing has been pushed to this branch and no fix is applied — this is a note, not a change request. Session-scoped
|
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