Repository navigation
ClosingPanes: the springing-close rules are Rust's (cmux_closing_panes_*); the Swift rule copy goes (step B of #35886) - #35934
lawrencecchen wants to merge 5 commits into
Conversation
…s_*); the Swift rule copy goes (step B) ClosingPanesRules keeps its API and now holds one cmux-layout-reducer closing_panes handle per screen: each call maps the gathered values into the C structs and the answer back. ClosingPanesCorpusTests replays the corpus the Swift rules recorded (answers and the whole state hash after each call) through the C calls. CmuxNextLayout links CCmuxAppFFI.
…_* call per method) and ClosingPanes MIXED 1 (the gather; its live-frame moving test stays Swift), rule (c) swift-logic-lines.py: swift_logic_lines 415,963 at e60197e -> 415,646 at this head (-317).
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
|
|
Review (independent review subagent): FIX FIRST at f6e8522 The code is correct. Two items block landing, and neither needs a push: a red caused by this PR, fixed by a line in the description, and the motion-frame bench, which has not finished. When both are done, this head can get SHIP if the bench shows nothing in the blocking list below. Blocking
Checks
Not blocking
Not run by me: no build, no test, no bench. I read the diff, the base Swift rules, the Rust |
…/closing-panes-step-b # Conflicts: # Packages/macOS/CmuxNext/Package.swift # plans/cmux-next/swift-logic-classes.tsv
…nswer (red) #35934 paired A/B on cmux21: the 4x2 Chromium column-close row's layout p95 was 4.20/4.28/4.45 ms on the head against 1.99/3.43/2.06 ms on the base. rects builds four fresh arrays and a dictionary per frame. The bound is the answer's dictionary, counted in Release.
…d allocates only its answer The live frames and row travels go into C arrays the rules handle keeps from frame to frame, the answer is read in place, `moving` is four comparisons instead of an array literal per live frame, and the row columns are deduplicated without a Set. Per frame this removes the FrameFact array, the CmuxClosingFrame array, the answer array and one array per live frame; the dictionary the screen reads is reserved once. Same calls into cmux-layout-reducer, same order: the corpus (ClosingPanesCorpusTests) replays through the same load path. Red first: ClosingPanesFrameAllocationTests (688add4).
Takes back #35886: step B replaces the Swift rule copy that #35886 recorded the corpus from with calls into the Rust port that #35886 added (planned two-step move; same answers, checked by the corpus through the FFI).
Summary
Step B of moving
ClosingPanes(FRAME OWNER S3c/S3d springing-close rules) into Rust. Step A #35886 (merged as e241a67) added the rules tocmux-layout-reducer(closing_panes) and the typed C calls tocmux-layout-reducer-ffi; their app FFI release is pinned on feat (#35911,cmux-app-ffi-728ed6cc8d33).ClosingPanesRules(ClosingPanes+Rules.swift) callscmux_closing_panes_*(one handle per screen;rects, the per-frame call, allocates nothing in Rust) and only maps the screen's geometry and live frames into the C structs and the answers back. The Swift copy of the rules is deleted.CmuxNextLayoutdepends onCCmuxAppFFI(Package.swift).ClosingPanesCorpusTestsnow checks the 160 recorded scripts (1,767 calls; each answer and a whole-state hash) through the FFI.ClosingPanesRulesPLATFORM, the reason names the calls;ClosingPanesMIXED 1, because its live-frame "moving" test stays in Swift.Owner: PL-motion-core (layout motion). Swift-logic plan move 3.
Numbers
scripts/cmux-next/swift-logic-lines.py, head a9a0b09 (this branch with feat ce9bdbc merged in) and its base ce9bdbc:The move is 420,966 to 420,399 (-567): -195 Swift lines deleted (the rule bodies less the new glue, same table), -372 reclassified by the two new rows (ClosingPanesRules PLATFORM, ClosingPanes MIXED 1: FFI calls and data mapping, lead rule (c)). Before the merge the same split held at d2a578b / f6e8522 (417,557, 417,362, 416,990).
Copy safety (review of #35886)
ClosingPanesRulesholds afinal class Handlethat owns the Rust handle (cmux_closing_panes_newin init,cmux_closing_panes_freein deinit), so copies of the value share one Rust state. No code copies it:ClosingPaneskeeps it asprivate let rules, and the onlyClosingPanesvalue isScreenContentView.closingPanes, which is mutated in place and replaced whole (closingPanes = ClosingPanes()in ScreenContentView+ClosingPanes.swift), never assigned to another variable (grepof Sources and Tests at this head). The corpus test makes a newClosingPanesRules()per script.Changelog
none
Observed / Not run
Observed (fleet, at the pre-merge head f6e8522; at the merge head a9a0b09 the compile check is job 9a8fa1b707bd42d6172671ea and the 5 suites are step 27b4140d30b5ddb79e7ffe71, results to follow):
cmux-ci compile-check --ref f6e852211b79… --app --package CmuxNext --recipe-version 2: job e6c8e6258fdc475bef80858b, "compile check CmuxNext: passed (310 s)", "compile check app: passed (111 s)", "compile check cmux-tui: passed (13 s)".cmux-ci run --class light --script scripts/ci/package-test-lane.sh --arg suite --arg Packages/macOS/CmuxNext --arg ClosingPanesCorpusTests,PaneCloseKeptHandleTests,ClosedPaneContentStaysUntilLandingTests,PaneContentPolicyTests,PaneGroundCoversPaneTests: step 77d85b5c414ef521ef70fef1 on aws-m4pro-7, "All 5 suites passed." (the corpus through the FFI).python3 scripts/cmux-next/prepush-gate.py: no new violations.Bench (motion-frame rows,
scripts/measure/split-spring.sh --suite=pane-perf, split, close and New Column in 2, 8 and 30 terminal layouts, base d2a578b vs this head on one worker, cmux-austin-mini-1):window_on_screen: noon both sides (a 1920x1080 display cannot hold the 1600x1000 STRESS window; Finder was frontmost) and frames run at 60 Hz, so missed frames on both sides are noise. Layout work per close frame, the one number that does not need a shown window, is equal: 4x2 p95 2.26 vs 2.20 ms, 6x5 p95 2.70 vs 2.94 ms.--vdisplay=120(a 1920x1200 virtual display at 120 Hz, window on screen on both sides), 3 rounds each, terminal rows: base step 3af0e9399da8d242ad1aabaa, head step 7acbff8501ad2603a82cec89, both on cmux-austin-mini-1.--browser, the top-left pane a Chromium page), same worker and display, 3 rounds each: base step adc5b0ffd3ef472b2451d832, head step 7166664787583cc83e6ddecf.Paired set 1 (lead order): cmux21, base and head alternated job by job
Base d2a578b, head f6e8522 (before the feat merge; the merge changed no ClosingPanes, corpus or cmux-layout-reducer file).
split-spring.sh --suite=pane-perf --config=bench --app-cache --vdisplay=120 --layouts=1x2,4x2,6x5, all on cmux21 (perf pool), in this order: terminal base e7ebea5dfef11aaf2573a580, head 3853057454b6205e9025ebf6, base a3fd69935e7aa7e9b745f4b5, head d10b15a7c919fa36d08eca4f (4 rounds each, close and New Column); Chromium (--browser, split, close and New Column, 3 rounds) base 17d1a1139a1ce276e638ef7a, head 9c058d5a89cd67e1c210e0f3. Every report:window_on_screen: yes, refresh 8.33 ms (1920x1200 virtual display at 120 Hz), no run error, nothing unmeasured. A pair is round i of a base job and round i of the head job after it. These refs predate #36027, so no round carries background daemon CPU and the "over 1 s dropped" table is the same as the table over all pairs.Missed frames per pair (head - base):
term: all pairs (pair = job/round)
chrome: all pairs (pair = job/round)
Per row over all rounds, with the review's three block tests (light rows only; frames p50/p95/p99 are the median of the rounds, max is over all rounds, layout is the per-frame layout pass, which holds the
rectscall):term: frames p50/p95/p99 (median of rounds) / max (all rounds), missed (sum), layout p95 (median) / max, ms
chrome: frames p50/p95/p99 (median of rounds) / max (all rounds), missed (sum), layout p95 (median) / max, ms
Paired set 2 (running): Chromium repeat and STRESS
Base ce9bdbc (feat), head a9a0b09 (this branch with that feat merged), cmux21, alternated: Chromium light 2 more jobs per arm (split, close, New Column; 3 rounds), STRESS terminal 1 job per arm and STRESS Chromium (
--layouts=stress --stress-selected=chromium) 1 job per arm, 3 rounds each. These refs carry #36027, so each round records background daemon CPU and the second table drops rounds over 1 s. The verdict uses the review's block tests on the Chromium close rows over 3 job pairs and on the STRESS rows.rectscall runs in the layout pass): close rows layout p95 base 1.65, 2.20, 3.38 ms; head 2.63, 2.19, 3.05 ms (1x2, 4x2, 6x5).Not run: no live dogfood of a close on a person's Mac.