From b370bb0564202152464aed4c155f7991c7066a18 Mon Sep 17 00:00:00 2001 From: Lee Dong Geon <100822592+ldg030201@users.noreply.github.com> Date: Fri, 6 Nov 2026 14:17:54 +0900 Subject: [PATCH] perf(debugger): send each file's breakpoints together and redraw the gutter less (#1679) At session start and around Force Step Over / Force Run to Cursor, setBreakpoints went out one file at a time, each waiting for the previous answer, so every file with breakpoints added a round trip before the program ran or the stop was handled. The requests now go out together, and files whose breakpoints are all disabled are skipped (a fresh adapter has none). The breakpoint gutter now rebuilds only when its own file's breakpoint states change or a session starts or ends, not on every step. Fixes #1677 Co-authored-by: ldg030201 <100822592+ldg030201@users.noreply.github.com> --- Cargo.lock | 1 + crates/ide-dap/Cargo.toml | 1 + crates/ide-dap/src/lib.rs | 73 +++++++++++++------ crates/ide-dap/src/tests.rs | 44 +++++++++-- .../debugger/__tests__/editor.test.ts | 42 ++++++++++- .../debugger/editor/breakpointGutter.ts | 11 ++- 6 files changed, 136 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69fe6374d..26c8f63b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4212,6 +4212,7 @@ dependencies = [ name = "ide-dap" version = "0.1.37" dependencies = [ + "futures", "ide-core", "ide-jsonrpc", "ide-language", diff --git a/crates/ide-dap/Cargo.toml b/crates/ide-dap/Cargo.toml index 9cb2f2a6c..757915cca 100644 --- a/crates/ide-dap/Cargo.toml +++ b/crates/ide-dap/Cargo.toml @@ -6,6 +6,7 @@ edition.workspace = true license.workspace = true [dependencies] +futures.workspace = true tempfile.workspace = true ide-core.workspace = true ide-language.workspace = true diff --git a/crates/ide-dap/src/lib.rs b/crates/ide-dap/src/lib.rs index 8428ac8d1..6971794a2 100644 --- a/crates/ide-dap/src/lib.rs +++ b/crates/ide-dap/src/lib.rs @@ -19,6 +19,7 @@ mod session; use adapter::{AdapterLauncher, AdapterSpec, ProcessLauncher}; use connection::{Connection, DapError}; +use futures::future::join_all; use ide_core::{DebugSessionId, IdeError, IdeResult, SharedSink}; use ide_language::LanguageRegistry; use parking_lot::Mutex; @@ -510,6 +511,18 @@ pub(crate) fn capabilities_from(caps: &Value) -> DebugCapabilities { } } +/// States of a file's line breakpoints that no session has been sent. +fn unsent(breakpoints: &[Breakpoint]) -> Vec { + breakpoints + .iter() + .map(|b| BreakpointState { + line: b.line, + verified: false, + message: (!b.enabled).then(|| "Disabled".to_owned()), + }) + .collect() +} + /// States for breakpoints no session has verified yet. fn unverified(enabled: impl Iterator) -> Vec { enabled @@ -877,12 +890,20 @@ impl DebugManager { let _ = tokio::time::timeout(INIT_TIMEOUT, initialized.wait_for(|v| *v)).await; } - let files: Vec<(PathBuf, Vec)> = - self.breakpoints.lock().iter().map(|(p, b)| (p.clone(), b.clone())).collect(); + // A fresh adapter has no breakpoints, so a file with none enabled needs no request; the + // others go out together, as VS Code sends them, instead of one round trip per file. + let (files, idle): (Vec<_>, Vec<_>) = self + .breakpoints + .lock() + .iter() + .map(|(p, b)| (p.clone(), b.clone())) + .partition(|(_, b)| b.iter().any(|b| b.enabled)); + let sent = join_all(files.iter().map(|(path, bps)| s.send_breakpoints(path, bps, None))).await; // Breakpoint states follow the owner's first session. let first = self.live_sessions().into_iter().find(|f| f.owner.key == s.owner.key).map(|f| f.id) == Some(s.id); - for (path, bps) in files { - match s.send_breakpoints(&path, &bps, None).await { + let idle = idle.into_iter().map(|(path, bps)| (path, Ok(unsent(&bps)))); + for (path, result) in files.into_iter().map(|(path, _)| path).zip(sent).chain(idle) { + match result { Ok(states) => { if first { self.states.lock().insert(path.clone(), states); @@ -990,16 +1011,7 @@ impl DebugManager { Err(e) => tracing::debug!("setBreakpoints on session {} failed: {e}", s.id.0), } } - let states = result.unwrap_or_else(|| { - breakpoints - .iter() - .map(|b| BreakpointState { - line: b.line, - verified: false, - message: (!b.enabled).then(|| "Disabled".to_owned()), - }) - .collect() - }); + let states = result.unwrap_or_else(|| unsent(&breakpoints)); self.states.lock().insert(path, states.clone()); Ok(states) } @@ -1327,13 +1339,26 @@ impl DebugManager { /// Sends empty breakpoint lists (line, function, data) to `session` until its next /// stop, when the stored ones are restored. `except` is left to the caller. async fn suspend_breakpoints(&self, session: &Arc, except: Option<&Path>) -> IdeResult<()> { - let files: Vec = self.breakpoints.lock().keys().cloned().collect(); - for path in files { - if Some(path.as_path()) == except { - continue; + // While paused the adapter holds the stored lists: only files with something enabled change. + let files: Vec = self + .breakpoints + .lock() + .iter() + .filter(|(path, bps)| Some(path.as_path()) != except && bps.iter().any(|b| b.enabled)) + .map(|(path, _)| path.clone()) + .collect(); + let sent = join_all(files.iter().map(|path| session.send_breakpoints(path, &[], None))).await; + let mut failed = None; + for (path, result) in files.into_iter().zip(sent) { + match result { + Ok(_) => { + session.pending_restore.lock().files.insert(path); + } + Err(e) => failed = failed.or(Some(e)), } - session.send_breakpoints(&path, &[], None).await?; - session.pending_restore.lock().files.insert(path); + } + if let Some(e) = failed { + return Err(e.into()); } if !self.function_breakpoints.lock().is_empty() && session.supports("supportsFunctionBreakpoints") { session.send_function_breakpoints(&[]).await?; @@ -1349,9 +1374,11 @@ impl DebugManager { /// Re-sends what [`suspend_breakpoints`](Self::suspend_breakpoints) / run to cursor replaced. pub(crate) async fn restore_breakpoints(&self, session: &Arc) { let pending = std::mem::take(&mut *session.pending_restore.lock()); - for path in pending.files { - let bps = self.stored_breakpoints(&path); - if let Err(e) = session.send_breakpoints(&path, &bps, None).await { + let files: Vec<(PathBuf, Vec)> = + pending.files.into_iter().map(|path| (path.clone(), self.stored_breakpoints(&path))).collect(); + let sent = join_all(files.iter().map(|(path, bps)| session.send_breakpoints(path, bps, None))).await; + for ((path, _), result) in files.iter().zip(sent) { + if let Err(e) = result { tracing::debug!("restoring breakpoints of {} failed: {e}", path.display()); } } diff --git a/crates/ide-dap/src/tests.rs b/crates/ide-dap/src/tests.rs index e86cb6fc9..d82a7e673 100644 --- a/crates/ide-dap/src/tests.rs +++ b/crates/ide-dap/src/tests.rs @@ -17,6 +17,8 @@ struct Script { start_debugging: bool, ignore_configuration_done: bool, source_path: Option, + /// Answer `setBreakpoints` only once this many are waiting (a client that waits for each never gets one). + hold_set_breakpoints: usize, } struct FakeLauncher { @@ -49,6 +51,7 @@ async fn fake_adapter(stream let mut r = BufReader::new(r); let mut seq = 1000; let mut pending_launch: Option = None; + let mut held: Vec = Vec::new(); macro_rules! send { ($v:expr) => {{ let mut v: Value = $v; @@ -101,14 +104,20 @@ async fn fake_adapter(stream send!(json!({"type": "event", "event": "initialized"})); } "setBreakpoints" => { - let bps: Vec = msg["arguments"]["breakpoints"] - .as_array() - .unwrap() - .iter() - .enumerate() - .map(|(i, b)| json!({"id": i + 1, "verified": true, "line": b["line"]})) - .collect(); - ok!(msg, json!({"breakpoints": bps})); + held.push(msg); + if held.len() < script.hold_set_breakpoints { + continue; + } + for msg in std::mem::take(&mut held) { + let bps: Vec = msg["arguments"]["breakpoints"] + .as_array() + .unwrap() + .iter() + .enumerate() + .map(|(i, b)| json!({"id": i + 1, "verified": true, "line": b["line"]})) + .collect(); + ok!(msg, json!({"breakpoints": bps})); + } } "setExceptionBreakpoints" => ok!(msg, json!({})), "setFunctionBreakpoints" | "setDataBreakpoints" => { @@ -921,6 +930,7 @@ async fn force_stepping_suspends_breakpoints_until_the_next_stop() { let b = PathBuf::from("/p/b.py"); h.manager.set_breakpoints(a.clone(), vec![bp(1)]).await.unwrap(); h.manager.set_breakpoints(b.clone(), vec![bp(3)]).await.unwrap(); + h.manager.set_breakpoints(PathBuf::from("/p/off.py"), vec![Breakpoint { enabled: false, ..bp(5) }]).await.unwrap(); h.manager.set_function_breakpoints(vec![function_bp("run", true)]).await.unwrap(); let id = h.start(config()).await.unwrap(); h.wait_event(|e| matches!(e, DebugEvent::Stopped { .. })).await; @@ -949,6 +959,24 @@ async fn force_stepping_suspends_breakpoints_until_the_next_stop() { h.manager.run_to(id, 1, a.clone(), 9, true).await.unwrap(); assert_eq!(sent_for("/p/a.py").last().unwrap(), &json!([{"line": 10}])); assert_eq!(sent_for("/p/b.py").last().unwrap(), &json!([])); + assert!(sent_for("/p/off.py").is_empty(), "nothing enabled there, nothing to suspend or restore"); +} + +#[tokio::test] +async fn startup_sends_every_file_at_once_and_skips_files_with_nothing_enabled() { + let h = harness(Script { hold_set_breakpoints: 2, ..Script::default() }); + h.manager.set_breakpoints(PathBuf::from("/p/a.py"), vec![bp(1)]).await.unwrap(); + h.manager.set_breakpoints(PathBuf::from("/p/b.py"), vec![bp(3)]).await.unwrap(); + let off = PathBuf::from("/p/off.py"); + h.manager.set_breakpoints(off.clone(), vec![Breakpoint { enabled: false, ..bp(5) }]).await.unwrap(); + // Waiting for each answer before the next request would stall until the request timeout. + tokio::time::timeout(Duration::from_secs(5), h.start(config())).await.expect("requests pipelined").unwrap(); + let paths: Vec = requests(&h, "setBreakpoints").into_iter().map(|r| r["source"]["path"].clone()).collect(); + assert_eq!(paths, [json!("/p/a.py"), json!("/p/b.py")]); + assert_eq!( + h.manager.breakpoint_states(&off), + [BreakpointState { line: 5, verified: false, message: Some("Disabled".into()) }] + ); } #[tokio::test] diff --git a/src/features/debugger/__tests__/editor.test.ts b/src/features/debugger/__tests__/editor.test.ts index dd42f3542..b95e467e8 100644 --- a/src/features/debugger/__tests__/editor.test.ts +++ b/src/features/debugger/__tests__/editor.test.ts @@ -3,8 +3,14 @@ import { EditorState } from "@codemirror/state"; import { EditorView, GutterMarker, gutter, lineNumbers } from "@codemirror/view"; import { breakpointGutter, lookOf } from "../editor/breakpointGutter"; import { executionPoint } from "../editor/execution"; -import { breakpointsOf, loadBreakpoints, toggleBreakpoint, updateBreakpoint } from "../breakpoints/store"; -import { adoptSession, useExecutionPoint, useDebug, handleDebugEvent } from "../state"; +import { + breakpointsOf, + loadBreakpoints, + toggleBreakpoint, + updateBreakpoint, + useBreakpoints, +} from "../breakpoints/store"; +import { adoptSession, useExecutionPoint, useDebug, handleDebugEvent, type DebugSession } from "../state"; import { getContext } from "@/platform/context"; import styles from "../editor/editor.module.css"; @@ -53,6 +59,38 @@ describe("breakpoint gutter", () => { }); }); +describe("breakpoint gutter redraws", () => { + it("follows a session starting and ending, not every update of a running one", async () => { + localStorage.clear(); + loadBreakpoints("/proj"); + useDebug.setState({ sessions: {} }); + const view = makeView("a\nb"); + toggleBreakpoint(ctx.path, 0); + const id = breakpointsOf(ctx.path)[0]!.id; + useBreakpoints.setState({ status: { [id]: { verified: true, message: null, line: 0 } } }); + await flush(); + expect(view.dom.querySelector(`.${styles.verified}`)).toBeNull(); + + const session = { id: 9, status: "running" } as DebugSession; + useDebug.setState({ sessions: { 9: session } }); + await flush(); + expect(view.dom.querySelector(`.${styles.verified}`)).not.toBeNull(); + + const reads = vi.spyOn(useBreakpoints, "getState"); + useDebug.setState({ sessions: { 9: { ...session, status: "paused" } } }); + useBreakpoints.setState({ + status: { [id]: useBreakpoints.getState().status[id]!, other: { verified: true, message: null, line: 3 } }, + }); + reads.mockClear(); + await flush(); + expect(reads).not.toHaveBeenCalled(); + + useDebug.setState({ sessions: { 9: { ...session, status: "terminated" } } }); + await flush(); + expect(view.dom.querySelector(`.${styles.verified}`)).toBeNull(); + }); +}); + describe("output class gutter", () => { it("disables ordinary, temporary and logging breakpoints and explains why", () => { loadBreakpoints("/proj"); diff --git a/src/features/debugger/editor/breakpointGutter.ts b/src/features/debugger/editor/breakpointGutter.ts index cb7447501..c094b88dd 100644 --- a/src/features/debugger/editor/breakpointGutter.ts +++ b/src/features/debugger/editor/breakpointGutter.ts @@ -186,14 +186,19 @@ export function breakpointGutter(ctx: EditorDocContext): Extension { private unsubs: Array<() => void>; private pending = false; private destroyed = false; + private running = sessionsRunning(); constructor(readonly view: EditorView) { this.unsubs = [ useBreakpoints.subscribe((s, prev) => { - if (s.byFile[path] !== prev.byFile[path] || s.status !== prev.status || s.muted !== prev.muted) - this.scheduleRebuild(); + // States arrive per file: another file's don't change this one's markers. + const states = (s.byFile[path] ?? []).some((b) => s.status[b.id] !== prev.status[b.id]); + if (s.byFile[path] !== prev.byFile[path] || states || s.muted !== prev.muted) this.scheduleRebuild(); }), + // The markers only show whether a session runs, while each step updates `sessions` several times. useDebug.subscribe((s, prev) => { - if (s.sessions !== prev.sessions) this.scheduleRebuild(); + if (s.sessions === prev.sessions || sessionsRunning() === this.running) return; + this.running = !this.running; + this.scheduleRebuild(); }), useSpecialBreakpoints.subscribe((s, prev) => { if (s.methods !== prev.methods || s.fields !== prev.fields || s.status !== prev.status)