monolith

public

Star0

Fork 0

Watch0

67 commits 1 branch 0 tags 0 releases1.5 MiB

Commit `9c7ef7f4bc24`

tarball

atagen <boss@atagen.co>2026-10-17 01:57 UTC

1 file changed+114-31

design: write up the full codebase review

Modified file: design/review-notes.md+114 additions−31 deletions@@ -1,42 +1,125 @@
-# Notes for the codebase review
+# Codebase review
 
-Collected while building M5 to M7 and the follow-ups. Each item names where to look. Ordered roughly by how much it would pay back.
+A read of every crate on 2026-10-17, after M5 to M7 and the M6 follow-ups. Findings are grouped by theme and ordered by payback within each group. The proposed order of work is at the end.
 
-## Structure
+## 1. The monolith crate has no internal boundaries
 
-- **`State` is a god object.** It has 70 public fields in `state.rs`. They cover backend handles, every smithay protocol state, layout, animation, capture, imports, Xwayland, lock, idle, activation and launch tracking. Every module is `impl State`, so nothing has a boundary and any function can touch anything. The protocol states are mostly inert after construction. They could live in one `Protocols` struct, with the live subsystems (lock, captures, imports, xwayland, toplevel handles) owning their own data and taking narrow arguments.
-- **Three copies of "draw an output".** `drm::render_surface`, `nested::render` and `capture::output_elements` each assemble cursor plus lock-or-scene, and each destructures `State` field by field to satisfy the borrow checker. `capture::output_elements` is the general version. The backends should call one function and keep only presentation.
-- **`scene::build` takes ten arguments** and carries `#[expect(clippy::too_many_arguments)]`. Most of them are one borrowed view of the world. A `SceneInput` struct, or the split above, removes it.
-- **`drm::render_surface` does too much.** It handles VRR switching, timing and budget, scene assembly, lock confirmation, capture damage, frame callbacks and the slow-frame log. VRR and stats could be methods on `Surface`.
-- **`handlers.rs` is a grab bag.** It holds compositor commit logic, xdg-shell, decorations, seat, selection, DnD, syncobj, `hold_until_readable`, workspace actions and fullscreen. The commit handler alone runs import, window commit, unmapped mapping, first-buffer, popups, layers and capture damage in sequence. A per-role dispatch would read better.
-- **`layout/src/lib.rs` is 1000 lines.** Tags, outputs, parking, scratchpads, focus history and arrangement share one `impl Layout`. Scratchpads and output parking look separable.
-- **`render-vk/src/renderer.rs` is 1100 lines.** It mixes the smithay trait impls, submission and stall handling, the shm cache, read-back and the new `DmabufImporter`.
+- **`State` (`state.rs:55`) has 70 public fields**, and all 22 modules are `impl State`. Nothing stops any function touching anything, and every new protocol adds a field and a `new()` line. The fields fall into a few groups:
+  - Inert protocol globals that only their own handler touches. These are the 20 or so `*_state` fields and `_dmabuf_global`.
+  - The backend: `host`, `drm`, `output_id`, `render_node`, `syncobj`.
+  - The window model, spread over seven maps (see below).
+  - Subsystems with their own state: `lock`, `captures`, `imports`, `xwayland`, `launches`, `ipc`, plus `inhibitors` and `inhibited`.
+  - Input: `suppressed`, `drag`, `pointer_pos`, `move_inverted`, `layer_focus`, `cursor_status`.
+  - Redraw flags: `needs_redraw`, `cursor_moved`.
+- **The backend is two `Option`s plus side fields.** `host: Option<Host>` and `drm: Option<Drm>` both exist. `output_id` only means something nested and `render_node` only on bare metal. `after_dispatch` calls both `nested::render` and `drm::render` every time, and each returns early when its option is `None`. A `Backend` enum, or a small trait for render, present and outputs, would make the split explicit.
+- **One window's state lives in seven places:**
+  - `windows`
+  - `last_kind`
+  - `awaiting`
+  - `placed_by_rule`
+  - `scene::Borders`
+  - `toplevels::ListHandles`
+  - the animator's tracks, glides and spawns
 
-## Duplication
+  `remove_window` (`state.rs:404`) cleans only some of them. `awaiting` and `last_kind` keep stale ids, and `ListHandles` is cleaned lazily in `publish_toplevels`. A per-window record (`Managed { window, kind, awaiting, by_rule, list_handle }`) would make removal one operation.
+- **`apply_layout` (`state.rs:469`, about 110 lines)** does six jobs: it collects the shown windows, detects kind switches for glides, retargets animations, configures clients, maps the space, then syncs focus and idle inhibit. It runs after every command, including `Spawn`, `Quit`, `Reload` and `Close`, through `commands.rs:166`.
+- **The commit handler (`handlers.rs:63`)** runs eight steps in sequence:
+  - the buffer handler
+  - early import
+  - the window's own commit
+  - capture damage
+  - first-commit mapping
+  - first-buffer detection
+  - popups
+  - layers
+  
+  It looks up the window by root surface twice, scanning `windows` and `unmapped` linearly each time. Routing on the surface's role first (toplevel, popup, layer, lock, cursor, X11) would make each path short.
 
-- `CLEAR` is defined identically in `drm.rs`, `nested/mod.rs` and `capture.rs`.
-- Closing a window (xdg `send_close` or X11 `close`) is written out in both `commands.rs` and `toplevels.rs`. It belongs on a window helper next to `root()` and `identity()`.
-- Window lookup is a linear scan: `window_id` by surface, `x11_window_id`, the toplevel key lookup in `toplevels.rs`, and `ListHandles::window`. That's fine at desktop window counts, but four spellings of the same search should be one index.
-- Smoke-test shell scripts were rewritten for every protocol (`/tmp/m4fix/smoke_*.sh`). A small `xtask` or `.nu` harness that starts nested monolith with a client list and collects logs would make protocol checks repeatable, and could gate CI later.
+## 2. Drawing an output exists three times
 
-## Code quality
+`drm::render_surface` (`drm.rs:585`, 140 lines), `nested::render` (`nested/mod.rs:169`, 135 lines) and `capture::output_elements` (`capture.rs:265`) each assemble the cursor plus lock-or-scene. Each destructures `State` field by field to get past the borrow checker. On top of that, `render_surface` handles VRR switching, budget accounting, slow-frame logging, lock confirmation, capture damage and frame callbacks. Frame callbacks are written twice, once in each backend, with the lock and layer variants on top.
 
-- **Per-dispatch work that doesn't need to run.** `after_dispatch` runs `publish_workspaces` and `publish_toplevels` after every batch of events. They rebuild full snapshots and lock every toplevel's data for titles even when nothing changed. A dirty flag set by commits, layout changes and focus changes would make them nearly free.
-- **Silent `let _ =`** on X11 `configure` and `set_*` calls, on `notifier.successful`, and on several protocol sends. Most are fine, but the X11 ones hide real failures.
-- `configure()` in `state.rs` handles both xdg and X11 windows with two inline branches. Once more window kinds exist (none planned), it wants a small trait.
-- **`Xwayland` lifecycle.** A SIGKILLed monolith leaves its `/tmp/.X*-lock` and socket behind. `claim` handles stale locks by pid, but a recycled pid makes it skip that display for good. Low priority.
-- `Focus::X11(Box<X11Surface>)` exists only to satisfy `large_enum_variant`. That's fine, but `X11Surface` is 432 bytes, which is worth knowing.
-- Comment density is uneven. Some modules carry a doc line on most items, others none. Check against the comment rules in one sweep.
+- One `frame_elements(world, output) -> Vec<Draw>` used by all three.
+- One `send_frame_callbacks(world, output, elapsed)`.
+- The backends keep pacing and presentation only.
+- `scene::build` takes ten arguments (`scene.rs:45`, `#[expect(clippy::too_many_arguments)]`). That disappears once it takes a borrowed view struct.
 
-## Tests
+**Correctness smell.** `anim.prune(now)` runs inside each output's render with that output's own `now` (`drm.rs:658`). With two outputs at different rates, the later one can prune state the earlier one is still drawing. It's also why `animating` only re-arms the output that noticed it. Pruning belongs once per dispatch, after every output has drawn.
 
-- **The layout proptest pulls its weight.** It caught the hidden root-leaf bug the moment the "hidden tiles have no area" invariant was added. More invariants in `check_geometry` (focus always visible, tags always viewed somewhere) would be cheap and high-yield.
-- **Nothing tests the protocol servers.** `workspace.rs` and `toplevel.rs` diff snapshots by hand, which is exactly the strange, racy logic the test policy wants covered. Candidates are a snapshot-diff test driving two `update` calls through a client, or a pure function that computes the diff and a test of that.
-- `render-vk` has golden and dmabuf tests. Nothing pins the submit-stall absorption logic, since `StallPhase::saw` is pure and tiny. It's a candidate only if it ever changes.
-- `monolith` itself has no tests, so behaviour such as lock confirmation, capture waiting and import blockers is only smoke-tested. Most of it needs a display. The pure parts (the frame budget, `floats()`, workspace filtering) could be pulled out and tested if they ever break.
+## 3. Snapshots for clients are rebuilt three ways, every dispatch
 
-## Smaller observations
+`ipc::broadcast`, `publish_workspaces` and `publish_toplevels` each derive a view of tags, windows and outputs from `Layout` plus `windows`. They run after every batch of events, and they lock every toplevel's role data for titles even when nothing changed.
 
-- `state.rs` free functions `root`, `identity`, `configure`, `gaps` and `xkb_config` are window, layout and config helpers living in the state module.
-- The `layers.rs` debug lines added to chase the quickshell report can be dropped or kept at trace.
-- `design/parked.md` and this file overlap a little. Parked is for behaviour and this is for structure.
+- A single derived model, computed only when a dirty flag is set by commits, layout and focus changes, would feed all three.
+- `slotmap::Key::data(&id).as_ffi()` appears in both `ipc.rs:177` and `toplevels.rs:26`.
+- Tag names are looked up by id with a linear `tags().find` twice in `ipc.rs`. `Layout` lacks a `tag(id) -> &Tag` accessor.
+
+## 4. Duplication
+
+- `CLEAR` is defined identically in `drm.rs:40`, `nested/mod.rs:32` and `capture.rs`.
+- Closing a window (xdg `send_close` or X11 `close`) is written in both `commands.rs:63` and `toplevels.rs:57`.
+- Window lookup is a linear scan in four places: `State::window_id`, `x11_window_id`, the key search in `toplevels.rs:42` and `ListHandles::window`.
+- Point-in-rect is written out inline in `drm.rs:787`, `drm.rs:645`, `capture.rs:270` and `state.rs` `surface_under`. The planner's `Rect` has no `contains`.
+- Rect helpers are scattered: `inset`, `centred` and `translate` in `layout/lib.rs:979`, `pixels` in `scene.rs:279`, `span` and `with_span` in `tree.rs`.
+- `Dir`, `Axis` and `ExpandScope` are defined in both `monolith-ipc` and `monolith-layout`, with hand-written mapping functions in `commands.rs:42`. `Modifier` and `Mods` from config are matched against smithay's `ModifiersState` by hand in two places (`commands.rs:29`, `input.rs:182`).
+- `protocols/src/workspace.rs` and `toplevel.rs` repeat the same skeleton: a manager list, a snapshot, a per-manager sync, bind-then-sync, and stop/destroyed pruning. If a third such protocol arrives, factor out the skeleton. With two, they're tolerable.
+- **Config defaults live in two places:** the `Default` impls in `config/src/lib.rs` and `default.toml`, 111 lines. The only test (`config/lib.rs:415`) checks that the file parses, not that it agrees with `Default`. `Raw` (`lib.rs:264`) mirrors `Config` field by field as well.
+
+## 5. Module placement
+
+- `state.rs` free functions belong elsewhere. `root`, `identity` and `configure` are window helpers. `gaps` and `xkb_config` are config adapters. `now_ns` is a clock.
+- `handlers.rs` mixes core compositor, xdg-shell, seat, selection, DnD, syncobj, decorations and workspace actions. The newer protocols each got their own module (`layers`, `lock`, `idle`, `capture`, `toplevels`). The older ones should follow so the file stops being a junk drawer.
+- `render-vk/src/renderer.rs` (1113 lines) holds:
+  - the smithay trait impls
+  - submission and the NVIDIA stall logic
+  - the 160-line `submit` that records all ops
+  - upload and read-back
+  - the shm cache
+  - `DmabufImporter`
+  - the image barrier helpers (`barrier`, `acquire`, `release` at `renderer.rs:690`)
+  
+  The barriers belong next to `sync.rs`. Stall handling and the shm cache would be self-contained modules.
+- `layout/src/lib.rs` (1008 lines) covers tags, outputs and parking, windows, focus history, scratchpads and arrangement in one `impl`. Scratchpads (`765–853`) and output parking (`225–339`) are the clean cuts.
+- `anim.rs` keeps `VulkanTexture` inside `Ghost`, which ties the animation model to the GPU. The ghost's texture could live in the scene, keyed by window, with the animator holding only geometry and timing.
+- The DRM frame pacing (`Budget`, `Surface::next_deadline`, `drm.rs:86–163`) and nested's `Host::predict` are pure timing logic stuck inside backend files.
+
+## 6. Code quality
+
+- **render-vk has 84 `unsafe` sites and no `// SAFETY:` comments.** The monolith crate does comment its few. `submit` keeps one `unsafe` block across about 120 lines of recording. Narrower blocks with an invariant stated per helper would make the Vulkan code reviewable.
+- **Silent `let _ =`** sits on X11 `configure` and `set_*` (`state.rs`, `xwayland.rs`), `notifier.successful`, `popup.send_configure`, `display.flush_clients` and protocol sends. The X11 ones can hide real failures, so log those at debug.
+- `commands::spawn` starts one OS thread per child just to `wait()` on it. A SIGCHLD source on the event loop reaps them all without threads.
+- `drm::on_input` maps absolute pointer motion onto the first output only (`drm.rs:751`). That's wrong for tablets and multi-monitor.
+- `Budget::ns` allocates and sorts 32 samples on every call, which is several times per frame. It's cheap, but a fixed array or a running quantile would do.
+- A SIGKILLed monolith leaves `/tmp/.X*-lock` and its socket behind (`xwayland.rs:31` cleans up only on `Drop`). `claim` recovers stale locks by pid, but a recycled pid wastes a display number.
+- Comment density is uneven across modules. Do one pass against the commenting rules.
+
+## 7. Tests: what earns its keep and what's missing
+
+What exists:
+- **planner:** 5 named strategic cases plus 3 proptests (random transitions, rewinding, interrupts). High value, low count. The custom `Rng` inside proptest is unusual but fine.
+- **layout:** a 2000-case proptest of random commands with `check_geometry`, plus two named tree cases. One invariant added this week found a real bug at once.
+- **motion:** five small curve and velocity tests.
+- **render-vk:** golden image tests, dmabuf round-trips and a transform test.
+- **config:** one parse test.
+- **monolith:** none.
+
+Gaps, by value:
+1. **Layout invariants are cheap and catch real bugs.** Add these to `check_geometry`:
+   - the focused window is visible
+   - every visible window's tag is viewed on its output, or the window is sticky or a shown scratchpad
+   - no window appears in two trees
+   - parked outputs restore their windows
+2. **Frame pacing.** `Budget` and `next_deadline` are pure, and they have been the source of most of the visible bugs: speed changes, periodic misses, slack decay. A small test that drives a timing sequence through them (a spike, then recovery, then miss and hold) would pin the behaviour that took several bare-metal rounds to find.
+3. **Protocol snapshot diffing** in `workspace.rs` and `toplevel.rs` is fiddly and has a history of hidden-state bugs. Pull the diff into a pure function from (sent, snapshot) to events and test it with the cases that matter: removal, output enter and leave, and a property change with no structural change.
+4. **Config:** assert that `default.toml` parses to the same values as `Default`, which closes the drift in section 4.
+5. **Nested smoke harness.** Every protocol this week was verified with a throwaway shell script that starts nested monolith with clients and greps logs. One `.nu` harness (or an `xtask`) that runs scenarios (layer-shell plus a client, lock, capture, toplevel list, Xwayland) would make those checks repeatable. It could run in `flake check` against lavapipe later.
+
+## Proposed order
+
+1. **Split `State`** into protocol globals, the backend enum, the window model with per-window records, subsystems and input. This unlocks most of the rest.
+2. **One output frame path:** element assembly and frame callbacks shared by both backends and capture, with pruning once per dispatch.
+3. **One derived snapshot with a dirty flag**, feeding ipc, ext-workspace and wlr-toplevel.
+4. **Mechanical dedup:** `CLEAR`, close, window index, `Rect::contains`, the shared enums between ipc and layout, the modifier mapping, and single-source config defaults.
+5. **File moves:** `handlers.rs` per protocol, the render-vk barriers, stall and shm cache modules, layout scratchpads and parking, frame pacing out of `drm.rs`.
+6. **Tests:** layout invariants, pacing, snapshot diffs, config defaults, then the smoke harness.
+7. **Hygiene sweep:** SAFETY comments, `let _ =` triage, the child reaper, absolute pointer mapping, and comments.

monolith

Commit 9c7ef7f4bc24

Commit `9c7ef7f4bc24`