From 2414095725a61526f56748bffff389058ae0f4f6 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 21:05:47 +0700 Subject: [PATCH] docs: Remove over-optimization action plan --- docs/over-optimization-action-plan.md | 299 -------------------------- 1 file changed, 299 deletions(-) delete mode 100644 docs/over-optimization-action-plan.md diff --git a/docs/over-optimization-action-plan.md b/docs/over-optimization-action-plan.md deleted file mode 100644 index ba0f447..0000000 --- a/docs/over-optimization-action-plan.md +++ /dev/null @@ -1,299 +0,0 @@ -# Over-optimization and over-engineering action plan - -- **Status:** phases 1 and 2 implemented; phase 3 still draft, for manual review -- **Date:** 2026-09-13 -- **Basis:** optimization inventory audit of branch `remove-ai-slop` (HEAD `534d572`) -- **Scope:** non-UI crates only — `signed_state`, `signed_git`, `signed_core`, `utils`, - `settings`, `paths`. -- **Moved out:** all UI-layer findings (workspace views, `signed_ui` components, dock chrome) - were removed from this plan and are tracked with the separate UI effort. See the appendix - for the mapping, so nothing is lost. -- **Owner decisions already made:** - - `BackendEvent::SyncProgress` **stays**. It is not removed by this plan. - - `Cargo.toml` build profiles and dependency feature selections are **out of scope**. - -Each task is written so it can be implemented and verified on its own. - -## Goals - -1. Remove speculative and dead machinery in the state/data layer that no behavior depends on. -2. Fix the places where an existing optimization is subtly incorrect or self-defeating. -3. Record, for every finding that is *not* acted on, why it is kept (see "Reviewed and kept"). - -## Non-goals - -- No UI work. Anything that changes rendering, view state, or widget behavior belongs to the - separate UI effort, even when the edit itself lands in a view file. -- No speculative performance work. Tasks in phase 3 are marked "measure first" and stay - optional until profiling or a concrete bug justifies them. -- No rewrite of the backend/store architecture. The coalescing model is kept; only its dead - state and one blocking sleep are touched. -- No dependency changes. In particular, no `smol`, no `ArcSwap`, no `indexmap`, no `dashmap`. - -## How to read a task - -- **Where** — files and current line numbers. Line numbers drift; re-locate symbols by name. -- **Size** — S (under an hour), M (half a day), L (a day or more). -- **Risk** — chance of a behavior change a user can notice. -- **Change** — what to do. -- **Acceptance** — how to know it is done. - -Implementers must follow the repository `.rules`: no `unwrap()` on fallible paths, propagate -errors where an operation can fail, full variable names, comments only for non-obvious "why", -and GPUI executor timers in tests when a test needs time to pass. - -## Baseline validation - -Run before and after every phase: - -```sh -cargo check --workspace -cargo test --workspace -``` - -Manual checks for phases 2 and 3 are in the validation section at the end. - ---- - -## Phase 1 — Trivial cleanups - -### OV-01 — Make `shorten_pubkey` panic-free and char-safe - -- **Where:** `crates/utils/src/pubkey.rs:4-14` -- **Evidence:** `to_bech32().unwrap()` and byte slicing `&npub[..9]` / `&npub[len-4..]`. - `crates/signed_ui/src/util.rs:4` has a char-safe `middle_truncate` with the same shape, but - moving it into `utils` would add a cross-crate dependency direction, so this task stays - local. -- **Change:** encode with `to_bech32().unwrap_or_else(|_| public_key.to_hex())` and truncate - by chars (collect to `Vec`, or iterate), leaving values too short for the ellipsis to - save space intact (the same `head + tail + ellipsis` guard as `signed_ui::middle_truncate`). - Output for normal keys must remain `npub1xxxx...yyyy`. -- **Acceptance:** add a unit test for a valid key and for a short fallback value; output of - the existing call site (`crates/signed_state/src/profile.rs:53`) is visibly unchanged. -- **Size:** S. **Risk:** low. -- **Status:** done 2026-09-13 — `crates/utils/src/pubkey.rs`, tests passing. - -### OV-02 — Align `docs/TODO.md` with the decision to keep `SyncProgress` - -- **Where:** `docs/TODO.md:3-9` -- **Evidence:** the TODO currently instructs a future session to delete the variant, the - `sync_progress` field, its accessor, and the progress task. That contradicts the owner - decision above. -- **Change:** replace that section with a short "kept intentionally" entry: the pipeline is - retained for a planned sync progress indicator; no subscriber exists yet. Do not touch the - `login`/`logout` section (separate decision). -- **Acceptance:** the file no longer instructs removal; `SyncProgress` code untouched. -- **Size:** S. **Risk:** low. -- **Status:** done 2026-09-13 — `docs/TODO.md` now marks the pipeline as kept intentionally. - ---- - -## Phase 2 — Runtime behavior - -### OV-03 — Simplify `RefreshGate` to running/dirty and give `CheckoutsStore` its own debounce flag - -- **Where:** `crates/signed_state/src/refresh.rs:1-56`; users at - `crates/signed_state/src/repos.rs:247,255,373,384`, - `crates/signed_state/src/repo.rs:300,312,455,554`, - `crates/signed_state/src/checkouts.rs:284,303,378,402,456,518`, and the one UI call site - `crates/workspace/src/views/inbox.rs:180-205,219,225,234`. -- **Evidence:** only `CheckoutsStore` ever leaves `debouncing` set across a call: it schedules - a 300 ms timer between `request()` and `begin()`. The other three users call `run_refresh` - synchronously, so the flag is set and cleared within one call. `inbox.rs:180` even asserts - the flag is false at its entry point. -- **Change:** - 1. Reduce `RefreshGate` to `running: bool` plus `dirty: bool`. `request()` returns - `Fold` while running, otherwise `Schedule`. Keep `running()`, `begin()`, `finish()` - (returns and clears `dirty`), `abort()` (keeps pending requests, matching today). - 2. Add `debounce_pending: bool` to `CheckoutsStore`. `refresh()` returns early when - `debounce_pending` or when `request()` returns `Fold`; otherwise it sets the flag and - spawns the existing `REFRESH_DEBOUNCE` timer. `run_refresh` clears the flag at its start. - `local_tick` and `run_local_statuses` check `refresh.running() || debounce_pending`. - 3. The other three users keep calling `run_refresh` directly after `Schedule`. - 4. The inbox view needs a mechanical adaptation only: delete the - `debug_assert!(!self.refresh.debouncing())` and replace it with a plain - `if self.refresh.running() { self.refresh.request(); return; }`. No rendering change. - If the UI effort is editing that file concurrently, coordinate the edit rather than - duplicating it. -- **Acceptance:** - - add unit tests in `refresh.rs` for: fold while running, follow-up after finish, abort - keeps the pending request, non-running request schedules; - - `cargo test -p signed_state`; - - manual: commit in a checkout, badges update within a few seconds; sidebar scan while - scanning coalesces; inbox refresh after a sync does not double-run. -- **Size:** M. **Risk:** medium. The debounce timing of `CheckoutsStore` must not regress; the - timer is preserved exactly, only its flag moves. -- **Status:** done 2026-09-13 — `RefreshGate` is `running`/`dirty` only, `CheckoutsStore` owns - `debounce_pending`, and the one-line `inbox.rs` adaptation landed here. - -### OV-04 — Replace the blocking sleep in the grasp push retry - -- **Where:** `crates/signed_state/src/backend.rs:1722-1726` - (`std::thread::sleep(GRASP_RETRY_DELAY)` inside `push_staged_to_grasps`) -- **Evidence:** the function is async and runs on GPUI's background executor. A blocking sleep - occupies a pool thread for up to two seconds per grasp server and can delay unrelated - background work. -- **Change:** await a GPUI executor timer for the same duration. `signed_state` has no - `smol` dependency and adding one is out of scope, but the executor is already reachable: - each caller captures `cx.background_executor().clone()` before `background_spawn` and - passes it into `push_staged_to_grasps` as a parameter, which then awaits - `executor.timer(GRASP_RETRY_DELAY)`. The accessor is already used this way at - `crates/signed_state/src/checkouts.rs:289`, and `BackgroundExecutor` is `Clone`. -- **Acceptance:** `cargo check -p signed_state`; a push that hits a transient denial still - retries with the same spacing. -- **Size:** S. **Risk:** low. -- **Status:** done 2026-09-13 — `push_staged_to_grasps` takes a `BackgroundExecutor` and awaits - `executor.timer(GRASP_RETRY_DELAY)`. - ---- - -## Phase 3 — Measure first (optional) - -These are optimization gaps, not over-engineering removals. Do not start any of them without -the measurement named in the task, and land them as separate PRs. - -### OV-05 — Batch the per-root status queries - -- **Where:** `crates/signed_state/src/repo.rs:390-396` issues one database query per root while - comments are batched at `:371-377`. `filters::statuses_for` already accepts many roots - (`crates/signed_core/src/filters.rs:83`). -- **Change:** collapse the loop into one query over all roots, mirroring the comment batching - above it. -- **Acceptance:** record refresh time on a repository with many issues/PRs before and after - the change; no change in resolved statuses (existing tests plus a manual comparison of a - repository with mixed open/closed/applied roots). -- **Size:** S. **Risk:** low. - -### OV-06 — Consolidate the per-checkout repository opens - -- **Where:** `crates/signed_state/src/checkouts.rs:598-668` calls five `signed_git` helpers per - checkout (`worktree_branches`, `worktree_dirty`, `worktree_current_branch`, - `head_commit_id`, `worktree_commits_ahead`), and each opens the repository separately. The - local poll runs every 2 s for up to `MAX_STATUS_CHECKOUTS` checkouts. -- **Change:** add a small `signed_git` API that opens the worktree once and returns the facts - the status computation needs (branches, dirty flag, current branch, head id, ahead count), - then use it from both status paths. Measure the poll cost before and after. -- **Acceptance:** checkouts tests in `crates/signed_state/src/checkouts.rs` pass; the - ready-to-push and ready-to-contribute statuses remain identical on a repository with a - dirty worktree, a clean feature branch, and a pushed branch. -- **Size:** M. **Risk:** medium (worktree state semantics). - -### OV-07 — Index URL matching in association resolution - -- **Where:** `crates/signed_state/src/checkouts.rs:573-593` is O(paths × announcements) with - repeated URL parsing on every full pass. -- **Change:** normalize and index the announcement clone URLs once per pass, then look up each - scanned origin instead of scanning all announcements. Keep the EUC match as a fallback. -- **Acceptance:** existing `resolve_associations` unit tests pass; record full-pass duration - on a large scan set before and after. -- **Size:** M. **Risk:** low to medium (URL identity rules are tested at `:754-777`). - -### OV-08 — Decide the commit total semantics - -- **Where:** `crates/signed_git/src/history.rs:159-190` caps materialization at - `MAX_LISTED_COMMITS` (20 000) but still walks all commits to compute `total`. -- **Change:** either accept the full walk and document it, or stop the walk at the cap and - expose whether the total is capped so callers can render `20000+`. The badge rendering - itself belongs to the UI effort; this task only changes the data layer and its contract. -- **Acceptance:** a decision is recorded here; if the cap is adopted, `CommitList` carries the - capped flag and the walk stops at the cap. Measure `worktree_all_commits` on the largest - available repository before deciding. -- **Size:** S to M. **Risk:** low. - ---- - -## Reviewed and kept (no action) - -These were flagged during the audit and reviewed; they should not be "fixed" without new -evidence: - -- `SyncProgress` pipeline — kept by owner decision (see OV-02). -- `Cargo.toml` release profile and dependency feature selection — out of scope. -- `GitCache` on-disk mirrors and `ensure_clone` fetch-on-open — core product behavior. -- The 64 MiB gix object cache for history walks and the plain opens for single-object reads. -- `CommitList` cap plus summary-only commits — memory bound is deliberate; the CPU question is - OV-08. -- Backend notification pump debounce and the day-quantized deletion filter — required for - sync dedup. -- `CheckoutsStore` polling design as a whole — revisit only via OV-06 and OV-07. -- `UniversalSigner` (`Arc>>`) — needed for in-place signer swap. -- Grasp transient-denial classification and the stale-advertisement convergence probe — - behavior justified by real races; only the blocking sleep is changed (OV-04). -- Debounced/batched profile sync (`ProfileStore`) and the typed filters in `signed_core`. - -## Validation - -Automated, per phase: - -```sh -cargo check --workspace -cargo test --workspace -``` - -Manual checks for the tasks that change runtime behavior: - -- **OV-03:** make a commit in a tracked checkout; the "ready to push" badge updates within a - few seconds. Push and confirm the badge clears without waiting for the next poll. Let a - repository scan overlap a manual rescan and confirm only one follow-up runs. -- **OV-04:** push to a grasp server that produces a transient denial (or simulate one) and - confirm the retry still happens at the same spacing and the push result is unchanged. -- **OV-05:** open a repository with many issues and PRs; statuses and counts match the - previous build. -- **OV-06/OV-07:** with several associated checkouts, confirm ready-to-push and - ready-to-contribute statuses match the previous build and the poll CPU cost drops. -- **OV-08:** open the Commits tab for a repository larger than the cap; the list behaves as - decided. - -## Suggested commit and PR breakdown - -Follow the repository PR hygiene rules: imperative titles, no conventional prefixes, a final -`Release Notes:` section with exactly one bullet. - -1. **`signed_state: Simplify refresh coalescing and stop blocking the executor`** - (OV-03, OV-04) - `Release Notes:` `- N/A` -2. **`utils: Make pubkey shortening panic-free`** (OV-01) - `Release Notes:` `- N/A` -3. **`docs: Record over-optimization decisions`** (OV-02 and the kept list) - `Release Notes:` `- N/A` - -OV-05 to OV-08 become separate PRs only after their measurement/decision step. - -## Appendix — moved UI tasks - -The following findings from the audit were removed from this plan and belong to the separate -UI effort. They are listed so they can be folded into that plan rather than lost. - -- Dock chrome: dead `add_bottom_panel`, unused visibility/scrollbar setters, `#[inline]` noise, - unreachable `Center` arm, duplicated `InvalidPanel` name, per-placement resize-handle - element ids (correctness), Linux window-controls decoration guard, per-frame layout walks, - never-set `tiles_scrollbar_mode`. -- Workspace views: file preview cache invariant drift, eager 1 MiB clone on cache hit, - `refs.rs` clone, repo header per-render announcement/share clones, eager share links, - About-dialog clone, PR row worktree clone, discussion participant dedup, Popular-sort - address recomputation, unbounded `tasks` vectors, duplicated close-panel helper, `defer_in` - for dock cleanup, PR alert state location, `open_pull_request` validation return value. -- `signed_ui`: per-render dropdown popover id allocation, pixel avatar RNG simplification. -- Retention/performance: `retain_all` image-cache memory profiling. - -Renumbering, for traceability from the first review round: - -| First-round ID | This plan | -|---|---| -| OV-08 | OV-01 | -| OV-09 | OV-02 | -| OV-10 | OV-03 | -| OV-11 | OV-04 | -| OV-30 | OV-05 | -| OV-31 | OV-06 | -| OV-32 | OV-07 | -| OV-34 | OV-08 | -| All other IDs | Moved to the UI effort (see above) | - -## Open questions for the owner - -1. OV-08: is an approximate (`20000+`) commit total acceptable for the badge contract? - -OV-03's questions were settled during implementation: the gate keeps two flags (`running`, -`dirty`), `CheckoutsStore` owns `debounce_pending`, and the mechanical `inbox.rs` edit landed -in this phase.